Add Bmotion tool (#12447)#12448
Conversation
WalkthroughThis PR adds the Bit.Bmotion Blazor animation library, its WebAssembly demo app, browser interop bridge, public animation models and services, routed demo pages, a README, and MSTest coverage for easing, transforms, colors, transitions, and animation drivers. ChangesBit.Bmotion end-to-end implementation
Sequence Diagram(s)sequenceDiagram
participant DemoPage
participant Bmotion
participant BmotionAnimationEngine
participant bit-bmotion.js
DemoPage->>Bmotion: render with Initial / Animate / gestures
Bmotion->>BmotionAnimationEngine: register and start animation
BmotionAnimationEngine->>bit-bmotion.js: startRafLoop / attach listeners
bit-bmotion.js->>BmotionAnimationEngine: ComputeFrame(timestamp)
BmotionAnimationEngine-->>bit-bmotion.js: CSS updates by element id
bit-bmotion.js-->>Bmotion: pointer / drag / viewport / scroll callbacks
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (19)
src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor-37-37 (1)
37-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
IsPresent="true"defeats AnimatePresence exit animations for list items.The
IsPresentprop is hardcoded totrue, which means the exit animation will never play when items are removed from the list. AnimatePresence needsIsPresentto toggle tofalsebefore the item is removed from the DOM.Consider binding
IsPresentto a dictionary that tracks pending removals, or refactor to wrap the entire@foreachloop so that AnimatePresence can detect membership changes in_items.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor` at line 37, The AnimatePresence usage currently hardcodes IsPresent="true", which prevents exit animations from running; change AnimatePresence/IsPresent so IsPresent is driven by state: either bind IsPresent to a boolean lookup (e.g., a Dictionary<Guid,bool> like pendingRemoval[item.Id]) and flip it to false before removing from _items, or instead move the AnimatePresence wrapper to surround the entire `@foreach` over _items so membership changes in _items are detected automatically; update the removal logic (the method that removes items) to first set pendingRemoval[item.Id] = false (or remove the item from whatever presence-tracking collection) and only after the exit animation completes actually remove the item from _items.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-145-148 (1)
145-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRotation property handling differs between serialization methods.
In
ToCssStyleString(), when bothRotateandRotateZare set, they're merged into a singlerotate()transform usingRotateZ ?? Rotate(Line 146). However, inToJsDictionary(), both properties are added separately if set (Lines 91, 94). This creates different behavior between server-side and client-side rendering, potentially causing a visual jump when JavaScript takes over.♻️ Proposed fix to align both methods
-if (Rotate.HasValue) d["rotate"] = Rotate.Value; +if (Rotate.HasValue && !RotateZ.HasValue) d["rotate"] = Rotate.Value;Or alternatively, emit both in CSS as well (though this may be less clear):
-if (Rotate.HasValue || RotateZ.HasValue) - transforms.Add($"rotate({RotateZ ?? Rotate}deg)"); +if (Rotate.HasValue && !RotateZ.HasValue) + transforms.Add($"rotate({Rotate.Value}deg)"); +if (RotateZ.HasValue) + transforms.Add($"rotate({RotateZ.Value}deg)");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 145 - 148, The server CSS and client JS disagree because ToCssStyleString() collapses Rotate and RotateZ into one rotate(...) transform while ToJsDictionary() currently adds both separately; update ToJsDictionary() so it mirrors ToCssStyleString() by adding a single rotate entry using RotateZ ?? Rotate (and avoid emitting both rotate and rotateZ keys), and ensure RotateX/RotateY handling remains unchanged; modify the logic around where rotate/RotateZ/Rotate are added in ToJsDictionary() to use the merged value (RotateZ ?? Rotate) and remove the duplicate emission of both properties.src/Bmotion/Bit.Bmotion/Components/Motion.cs-498-499 (1)
498-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Viewport.Onceprecedence is not respected on leave.Line 498 checks only
!Once. WhenViewportis set, docs sayViewport.Onceshould win; deactivation should use the effective once value (Viewport?.Once ?? Once).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/Motion.cs` around lines 498 - 499, The leave handler currently checks only the local Once flag; change it to use the effective once value from Viewport if present: replace the condition that uses !Once with one that evaluates !(Viewport?.Once ?? Once) so the deactivation call in the block (await Engine.DeactivateGestureLayerAsync(_id, "inview")) respects Viewport.Once; reference symbols: WhileInView, Once, Viewport, Engine.DeactivateGestureLayerAsync, _id, "inview".src/Bmotion/Bit.Bmotion/Services/MotionValue.cs-32-33 (1)
32-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnumerate a subscriber snapshot before awaiting callbacks.
At Lines 32–33, callbacks can subscribe/unsubscribe while iterating, which can throw
InvalidOperationExceptionand stop notification flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs` around lines 32 - 33, The loop over _subscribers in the MotionValue notification method can throw InvalidOperationException if subscribers change during iteration; fix by enumerating a snapshot (e.g., create a copy of _subscribers via ToList() or ToArray()) into a local variable and iterate over that snapshot when awaiting each subscriber (use the same local name like subscribers and await each sub(value)), ensuring you reference the same method in MotionValue that currently contains "foreach (var sub in _subscribers) await sub(value);".src/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cs-45-48 (1)
45-48:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent exceptions on invalid hex colors in
Parse.At Lines 45–48, malformed hex input throws instead of returning
null. That breaksLerp’s fallback behavior (return to) and can interrupt animation updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cs` around lines 45 - 48, The Parse method currently assumes the hex substrings are valid and uses Convert.ToInt32(h[..2], 16) etc., which throws on malformed input; update Parse to first validate h.Length (must be >=6 and <=8) and then use safe parsing (int.TryParse or equivalent with NumberStyles.HexNumber and CultureInfo.InvariantCulture) for each substring used in Convert.ToInt32(h[..2], 16), Convert.ToInt32(h[2..4], 16), Convert.ToInt32(h[4..6], 16) and the optional alpha slice (h[6..8]); if any TryParse fails, return null so Lerp’s fallback behavior remains intact. Ensure the alpha branch still divides by 255.0 when parsing succeeds.src/Bmotion/Bit.Bmotion/Services/MotionValue.cs-67-82 (1)
67-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate transform ranges for minimum length and segment validity.
Current checks only compare lengths. With fewer than 2 points, Line 81 can index out of range; with repeated adjacent input points, Line 77 can divide by zero.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs` around lines 67 - 82, The Map function's validation is insufficient: before using inputRange and outputRange, ensure they each have at least 2 points (inputRange.Length >= 2) and that inputRange is strictly monotonic (e.g., for all i inputRange[i+1] != inputRange[i], ideally inputRange[i+1] > inputRange[i]) to avoid index issues and division by zero in the loop inside Map; if these checks fail throw ArgumentException with a clear message. Add these validations alongside the existing length-equality check (referencing inputRange, outputRange and the local Map(T v) function) so the loop and the denominator (inputRange[i + 1] - inputRange[i]) are guaranteed non-zero. Ensure error messages mention which condition failed (too few points or non-increasing/repeated input range points).src/Bmotion/Bit.Bmotion/Services/MotionValue.cs-25-25 (1)
25-25:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid fire-and-forget async work in
Valuesetter.Line 25 drops the
SetAsynctask, so subscriber exceptions become unobserved and rapid sets can interleave unpredictably. This needs an explicit update model (e.g., async-only setter path or synchronized queue).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs` at line 25, The Value property's setter currently does a fire-and-forget call to SetAsync (set => _ = SetAsync(value)), causing unobserved exceptions and race conditions; change the update model by removing the async fire-and-forget from the setter: either make the property setter synchronous and call a synchronous helper (e.g., implement a SetSync method that awaits or locks and updates the backing field), or mark the setter obsolete/private and expose an explicit async API that callers must await (keep public Task SetAsync(T value) as the single update path); if concurrency is a concern, serialize updates inside SetAsync using a SemaphoreSlim or a dedicated request queue to ensure ordered, awaited execution and to surface exceptions from SetAsync to callers (refer to SetAsync and the Value property to apply these changes).src/Bmotion/Bit.Bmotion/Components/Motion.cs-112-116 (1)
112-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
AdditionalAttributesfrom overriding the generated element id.At Lines 112–116, an incoming
idinAdditionalAttributescan replace_id, breaking the engine/interop contract that targets this element by_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/Motion.cs` around lines 112 - 116, AdditionalAttributes can contain an "id" that overrides the generated _id; before calling builder.AddMultipleAttributes(seq++, AdditionalAttributes) remove any "id" key from AdditionalAttributes (or pass a filtered collection that excludes the "id" key) so the previously set builder.AddAttribute(seq++, "id", _id) remains authoritative; update the code around the builder.AddAttribute / builder.AddMultipleAttributes calls to filter out the "id" entry (e.g. call AddMultipleAttributes with AdditionalAttributes.Where(kvp => kvp.Key != "id") or copy and Remove("id")).src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js-124-129 (1)
124-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
unregisterElementreferences an undefined viewport observer variable.Line 128 uses
_vpObserver, but only_vpObserversexists. This throws during unregister/teardown and leaves viewport tracking state partially cleaned.Suggested patch
- if (_vpObserver && el) _vpObserver.unobserve(el); + if (el) _vpObservers.forEach(obs => obs.unobserve(el));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js` around lines 124 - 129, The unregisterElement function uses a non-existent _vpObserver variable; fix it to use the actual _vpObservers collection: retrieve the observer for the element via _vpObservers.get(elementId), call observer.unobserve(el) if both observer and el exist, and then remove that observer entry from _vpObservers (in addition to the existing _vpRefs.delete and _runCleanup calls) so viewport tracking is fully cleaned up.src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js-218-236 (1)
218-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPan delta is always zero due state update order.
At Lines 225 and 234–235,
lastX/lastYare overwritten before delta is emitted, soOnPanMovereceivesdeltaX=0anddeltaY=0every time.Suggested patch
const onMove = (e) => { const dx = e.clientX - startX, dy = e.clientY - startY; + const deltaX = e.clientX - lastX, deltaY = e.clientY - lastY; const now = Date.now(), dt = now - lastT; if (dt > 0) { - velX = (e.clientX - lastX) / dt * 1000; - velY = (e.clientY - lastY) / dt * 1000; + velX = deltaX / dt * 1000; + velY = deltaY / dt * 1000; } - lastX = e.clientX; lastY = e.clientY; lastT = now; + lastX = e.clientX; lastY = e.clientY; lastT = now; @@ dotnetRef.invokeMethodAsync('OnPanMove', e.clientX, e.clientY, - e.clientX - lastX, e.clientY - lastY, + deltaX, deltaY, e.clientX - startX, e.clientY - startY, velX, velY);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js` around lines 218 - 236, In onMove, the code updates lastX/lastY/lastT before calling dotnetRef.invokeMethodAsync('OnPanMove'), which makes the emitted delta values zero; compute deltaX and deltaY (e.g., const deltaX = e.clientX - lastX; const deltaY = e.clientY - lastY) and compute velX/velY using lastX/lastY/lastT first, then call OnPanMove with those computed deltaX/deltaY and velX/velY, and only after the invoke update lastX = e.clientX; lastY = e.clientY; lastT = now (or alternatively move the existing lastX/lastY/lastT assignments to after the invoke).src/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cs-70-76 (1)
70-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the cubic-bezier derivative used by Newton iteration.
At Line 73,
dbxomits thex2contribution in the final term, so the solver can converge to an incorrectuand return wrong easing values for some curves.Suggested patch
- double dbx = 3 * (1 - u) * (1 - u) * x1 + 6 * u * (1 - u) * x2 - 6 * u * (1 - u) * x1 + 3 * u * u; + double dbx = 3 * (1 - u) * (1 - u) * x1 + + 6 * u * (1 - u) * (x2 - x1) + + 3 * u * u * (1 - x2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cs` around lines 70 - 76, The Newton iteration's derivative calculation for the cubic-bezier is incorrect; replace the dbx assignment in the loop (the line that assigns to dbx using variables u, x1, x2) with the true derivative of x(u): dbx = 3 * ( (1 - u) * (1 - u) * x1 + 2 * (1 - u) * u * x2 + u * u ); this uses the correct contributions from x1 and x2 during the Newton step so the solver in the loop converges to the right u.src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs-49-55 (1)
49-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SetInstantchanges can be lost before they are emitted as CSS updates.Line 51 returns
nullwhen there are no active drivers, and Line 54 clears_dirtyPropseach tick. That drops dirty-only updates created bySetInstant(Lines 147-173), so instant style changes may never reach output.Suggested fix
- if (_activeAnims.Count == 0 && !_isDragging) return null; - - _transformDirty = _isDragging; // drag always refreshes transform - _dirtyProps.Clear(); + if (_activeAnims.Count == 0 && !_isDragging && !_transformDirty && _dirtyProps.Count == 0) + return null; + + var pendingProps = _dirtyProps.Count > 0 ? _dirtyProps.ToArray() : Array.Empty<string>(); + bool pendingTransformDirty = _transformDirty; + + _transformDirty = _isDragging || pendingTransformDirty; // preserve pending instant updates + _dirtyProps.Clear(); + foreach (var prop in pendingProps) _dirtyProps.Add(prop);Also applies to: 74-74, 147-173
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs` around lines 49 - 55, The Tick method currently returns null early and clears _dirtyProps each tick which causes SetInstant changes (created by SetInstant) to be dropped before being emitted; modify Tick (method Tick) so it only returns null when there are no active drivers, not dragging, AND _dirtyProps is empty (or alternatively move _dirtyProps.Clear to after emission), ensuring _dirtyProps is preserved until the CSS updates are emitted for SetInstant; update the logic around the early return and the placement of _dirtyProps.Clear so SetInstant-produced dirty properties survive the tick that emits them.src/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cs-27-27 (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard invalid inertia time constants before integration.
At Line 27,
_timeConstantSeccan be<= 0. Then Line 55 divides by it insideMath.Exp, which can produceNaN/Infinityand prevent convergence.Suggested fix
- _timeConstantSec = config.TimeConstant / 1000.0; + _timeConstantSec = Math.Max(config.TimeConstant / 1000.0, 0.001);Also applies to: 55-55
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cs` at line 27, The code assigns _timeConstantSec = config.TimeConstant / 1000.0 and later uses it inside Math.Exp causing NaN/Infinity if <= 0; add a guard in the InertiaDriver constructor (where _timeConstantSec is set) to validate config.TimeConstant > 0 and either throw ArgumentException or replace with a small positive epsilon (e.g. 1e-6) to avoid division by zero, and also add a defensive check right before the Math.Exp usage (the integration/update method that computes the exp factor) to handle non-positive _timeConstantSec by using the same epsilon or early-return behavior; reference the field _timeConstantSec and the integration/update method that computes Math.Exp so both assignment and usage are fixed.src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs-119-128 (1)
119-128:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNew
AnimateTocalls can strand previous completion awaiters.Line 127 overwrites
_completionSourceunconditionally. If another animation starts before the prior one finishes, the oldTaskCompletionSourcemay never complete.Suggested fix
- _completionSource = completionSource; + if (_completionSource != null && !ReferenceEquals(_completionSource, completionSource)) + _completionSource.TrySetCanceled(); + _completionSource = completionSource;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs` around lines 119 - 128, The AnimateTo method overwrites the field _completionSource unconditionally, which can leave previously supplied TaskCompletionSource instances never completed; before assigning _completionSource in AnimateTo, check if an existing _completionSource is present and complete it (e.g., call TrySetCanceled or TrySetResult) to ensure awaiting callers aren't stranded, then assign the new completion source; update any related cleanup logic that completes _completionSource on animation end to handle the new behavior so no double-completion or race occurs.src/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cs-97-101 (1)
97-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
onCompletetask is not properly awaited/observed.At Line 100,
ContinueWith(_ => onComplete(), ...)creates a nested task chain that is dropped. Exceptions fromonCompletecan become unobserved.Suggested fix
- _ = tcs.Task.ContinueWith(_ => onComplete(), TaskScheduler.Default); + _ = Task.Run(async () => + { + await tcs.Task.ConfigureAwait(false); + await onComplete().ConfigureAwait(false); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cs` around lines 97 - 101, The continuation created with tcs.Task.ContinueWith(_ => onComplete(), ...) drops the nested task so exceptions from onComplete go unobserved; fix by awaiting the continuation instead of dropping it — replace the ContinueWith call with explicit awaits (await tcs.Task; await onComplete();) after EnsureLoopRunningAsync() so the onComplete() Task is awaited and any exceptions are observed (refer to the local tcs variable, the onComplete delegate, and the EnsureLoopRunningAsync() call).src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs-30-66 (1)
30-66:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
ExitBeforeEnteris currently a no-op.The parameter is publicly documented, but the enter/exit state transitions never read it, so behavior is identical whether it is
trueorfalse. Please either implement the delayed-enter branch or remove the parameter until it is supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs` around lines 30 - 66, The ExitBeforeEnter parameter is unused; update the lifecycle logic in OnParametersSet and the OnAllExitsComplete handler so that when ExitBeforeEnter is true new children do not render until exiting children finish: in OnParametersSet, when transitioning from present->not present keep current behavior, but when transitioning from not present->present check ExitBeforeEnter — if true set a flag (e.g., _deferEnter = true) and do not set _shouldRender = true, otherwise proceed with current re-enter branch (set _presenceCtx.IsExiting = false, _presenceCtx.Reset(), _shouldRender = true); in OnAllExitsComplete (the method subscribed in OnInitialized) clear _presenceCtx.IsExiting, set _deferEnter = false and set _shouldRender = true so entering children render, and ensure _prevIsPresent is updated appropriately after these transitions so state remains consistent.src/Bmotion/Bit.Bmotion/Services/AnimationControls.cs-35-41 (1)
35-41:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Complete()does not implement its documented behavior.The method claims to snap elements to end values, but it currently performs the same operation as
Stop()(snap to current/intermediate values). This breaks the public API contract and should be implemented distinctly (or the contract/docs adjusted to match actual behavior).src/Bmotion/Bit.Bmotion/Services/ScrollTracker.cs-24-56 (1)
24-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRepeated
ObserveAsynccalls accumulate observers but overwrite callback state.Each new observe call replaces
_onScrollbut keeps previous subscriptions active. This can cause duplicate event streams and unexpected callback routing over time. Either enforce single active subscription (replace old before adding new) or track callbacks per subscription key.Proposed fix (single-active-subscription semantics)
public async Task ObserveAsync(string? containerId, Func<ScrollInfo, Task> onChange) { ObjectDisposedException.ThrowIf(_disposed, this); + + foreach (var key in _subscriptionKeys) + await _interop.UnobserveScrollAsync(key); + _subscriptionKeys.Clear(); + _onScroll = onChange; var key = await _interop.ObserveScrollAsync(containerId, _dotnet!); if (key != null) _subscriptionKeys.Add(key); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/ScrollTracker.cs` around lines 24 - 56, The ObserveAsync method currently overwrites _onScroll while leaving previous subscriptions in _subscriptionKeys active; change ObserveAsync (and use the corresponding MotionInterop unobserve method) to first remove any existing subscriptions before adding the new one: call the interop unobserve function for each key in _subscriptionKeys (e.g., UnobserveScrollAsync or the appropriate counterpart on MotionInterop) and clear _subscriptionKeys, then set _onScroll to the new callback and add only the freshly returned key; ensure ObjectDisposedException checks and _dotnet usage remain unchanged and that you only keep a single active subscription key in _subscriptionKeys.src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs-64-75 (1)
64-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
OnAllExitsCompleteagainst stale callbacks after re-entry.After
Reset()on re-entry, lateNotifyExitComplete(...)calls from prior exiting children can still triggerAllExitsComplete, and this handler then forces_shouldRender = falseeven when content is present again.Proposed fix
public partial class AnimatePresence : ComponentBase { + private bool _awaitingExitCompletion; protected override void OnParametersSet() { if (_prevIsPresent && !IsPresent) { _presenceCtx.IsExiting = true; + _awaitingExitCompletion = true; _shouldRender = true; } else if (!_prevIsPresent && IsPresent) { _presenceCtx.IsExiting = false; _presenceCtx.Reset(); + _awaitingExitCompletion = false; _shouldRender = true; } _prevIsPresent = IsPresent; } private void OnAllExitsComplete() { + if (!_awaitingExitCompletion || IsPresent) return; + _awaitingExitCompletion = false; _shouldRender = false; _presenceCtx.IsExiting = false; InvokeAsync(StateHasChanged); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs` around lines 64 - 75, OnAllExitsComplete must ignore stale callbacks after a re-entry: add an early-return guard at the top of OnAllExitsComplete that checks the current presence context state (for example, if (!_presenceCtx.IsExiting) return;) so that after Reset() or re-entry late NotifyExitComplete calls do not flip _shouldRender to false; leave the rest of the method (setting _shouldRender = false, _presenceCtx.IsExiting = false, InvokeAsync(StateHasChanged)) unchanged.
🟡 Minor comments (9)
src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.css-5-7 (1)
5-7:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider providing a visible focus indicator for accessibility.
Removing
outline: noneonh1:focuseliminates the keyboard navigation focus indicator. Users navigating with keyboard need visual feedback when focus moves to headings. Consider replacing with a custom focus style instead of removing it entirely, e.g.,outline: 2px solid#0071c1; outline-offset: 2px;.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.css` around lines 5 - 7, The h1:focus rule currently removes the keyboard focus indicator; update the CSS rule for h1:focus (or use h1:focus-visible) to provide a visible focus style instead of outline: none — for example set outline: 2px solid `#0071c1` and outline-offset: 2px (or a similarly high-contrast, accessible color/width) so keyboard users see clear focus on headings.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-158-160 (1)
158-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing OutlineColor to CSS string serialization.
OutlineColoris serialized inToJsDictionary()(Line 102) but is missing fromToCssStyleString(). This creates an inconsistency where server-side rendered initial styles won't include outline color, potentially causing a flash of unstyles content.🎨 Proposed fix
if (BorderColor != null) sb.Append($"border-color:{BorderColor};"); +if (OutlineColor != null) sb.Append($"outline-color:{OutlineColor};"); if (Fill != null) sb.Append($"fill:{Fill};");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 158 - 160, The CSS serializer ToCssStyleString is missing the OutlineColor property causing inconsistency with ToJsDictionary; update the ToCssStyleString method (near the lines that append BorderColor, Fill, Stroke) to append OutlineColor when set (e.g., if (OutlineColor != null) sb.Append($"outline-color:{OutlineColor};");) so server-side rendered styles include outline color just like ToJsDictionary does.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-163-163 (1)
163-163:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing BoxShadow to CSS string serialization.
BoxShadowis serialized inToJsDictionary()(Line 108) but is missing fromToCssStyleString(). This inconsistency means server-side rendered initial styles won't include box shadows.🎨 Proposed fix
if (BorderRadius != null) sb.Append($"border-radius:{BorderRadius};"); +if (BoxShadow != null) sb.Append($"box-shadow:{BoxShadow};"); if (PathLength.HasValue)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` at line 163, ToCssStyleString() is missing serialization for the BoxShadow property (it’s present in ToJsDictionary()), so add a check similar to BorderRadius that appends the CSS box-shadow rule; specifically, in ToCssStyleString() add something like: if (BoxShadow != null) sb.Append($"box-shadow:{BoxShadow};"); so server-rendered CSS includes the same box shadow output as ToJsDictionary().src/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csproj-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorUpdate .NET 10.0 availability concern.
Bit.Bmotion.Tests.csprojtargetsnet10.0;net9.0;net8.0. .NET 10 (net10.0) is publicly released, so the “may not be available yet” concern is not valid. Keep/ensure CI and dev machines have a .NET 10 SDK installed (e.g., SDK version shipped with the current .NET 10 servicing, such as 10.0.301).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csproj` at line 4, The TargetFrameworks element in Bit.Bmotion.Tests.csproj should retain net10.0 (i.e., keep "net10.0;net9.0;net8.0") because .NET 10 is publicly released; remove any comment or conditional that treats net10.0 as unavailable and ensure the TargetFrameworks entry includes net10.0. Also update CI/dev environments to install/use a .NET 10 SDK (for example 10.0.301) so builds and tests can run against the net10.0 target.src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs-10-11 (1)
10-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix documentation/code inconsistency.
The comment on Line 10 states "Default: false" but the property initializer on Line 11 sets
Enabled = true. These are inconsistent.Since this is an opt-in options class (users only provide
LayoutOptionswhen they want layout animations), havingEnabled = trueby default makes sense. Please update the comment to match the code:- /// <summary>Enable automatic layout animations. Default: false.</summary> + /// <summary>Enable automatic layout animations. Default: true.</summary> public bool Enabled { get; set; } = true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs` around lines 10 - 11, The XML doc for the LayoutOptions.Enabled property is incorrect; change its summary to reflect the actual default (true) by updating the comment on the Enabled property so it reads "Default: true." Target the property named Enabled on the LayoutOptions class and only modify the <summary> text to match the initializer (public bool Enabled { get; set; } = true;).src/Bmotion/Bit.Bmotion/Models/TransitionConfig.cs-183-213 (1)
183-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProperties dictionary is shallow-copied, not deep-cloned.
Line 211 copies the
Propertiesdictionary reference rather than creating a new dictionary. If code mutates the cloned config'sProperties, it will affect the original. This may violate the stated intent on lines 178-181 to avoid "mutating the original's fields."🛡️ Proposed fix to deep-clone the Properties dictionary
DelayChildren = DelayChildren, When = When, - Properties = Properties, + Properties = Properties == null ? null : new Dictionary<string, TransitionConfig>(Properties.Select(kv => new KeyValuePair<string, TransitionConfig>(kv.Key, kv.Value.Clone()))), OnUpdate = OnUpdate,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/TransitionConfig.cs` around lines 183 - 213, TransitionConfig.Clone currently shallow-copies the Properties dictionary (Properties = Properties) which lets mutations on the clone affect the original; change Clone to create a new dictionary instance and copy each entry into it, deep-cloning common mutable value types where appropriate (e.g., if a value is an array clone it with ((T[])value).Clone(), if it implements ICloneable call Clone(), otherwise copy the value reference), then assign that new dictionary to the Properties property; update the Clone method in class TransitionConfig to use this new dictionary so the cloned config has its own independent Properties collection.src/Bmotion/Bit.Bmotion/Components/Motion.cs-581-581 (1)
581-581:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTap listener activation should include
OnTapCancel.Line 581 enables tap listeners for
WhileTap/OnTapStart/OnTaponly. If onlyOnTapCancelis provided, tap events are never attached.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/Motion.cs` at line 581, The tap listener activation condition in Motion.cs currently checks WhileTap, OnTapStart, and OnTap only, so add OnTapCancel to the predicate so tap handlers are enabled when only OnTapCancel is provided; update the conditional that sets d["tap"] (the line referencing WhileTap, OnTapStart, OnTap) to also include OnTapCancel.src/Bmotion/Bit.Bmotion/Services/AnimationController.cs-44-47 (1)
44-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle
nullpassed toStop(params string[] properties).If a caller passes
nullexplicitly,properties.Lengththrows. This should be treated the same as “no properties”.Proposed fix
public void Stop(params string[] properties) { if (_elementId == null) return; - _engine.Stop(_elementId, properties.Length > 0 ? properties : null); + _engine.Stop(_elementId, properties is { Length: > 0 } ? properties : null); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/AnimationController.cs` around lines 44 - 47, The Stop method can throw when a caller explicitly passes null for the params array; update AnimationController.Stop to treat a null properties argument the same as no properties by checking properties == null || properties.Length == 0 before calling _engine.Stop so you pass null when there are no properties, e.g. compute a local props variable (or conditional) and call _engine.Stop(_elementId, props); keep the existing early return for _elementId and reference the Stop method and _engine.Stop call when making the change.src/Bmotion/Bit.Bmotion/Components/MotionConfig.razor-28-37 (1)
28-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
TransitionSpeedto prevent invalid negative durations.
TransitionSpeedis applied as a multiplier downstream; negative values can produce invalid transition timings. Guard this at the config boundary.Proposed fix
protected override void OnParametersSet() { + if (TransitionSpeed < 0) + throw new ArgumentOutOfRangeException(nameof(TransitionSpeed), "TransitionSpeed must be >= 0."); + _ctx.DefaultTransition = Transition; _ctx.ReduceMotion = ReduceMotion; _ctx.TransitionSpeed = TransitionSpeed; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/MotionConfig.razor` around lines 28 - 37, OnParametersSet currently assigns TransitionSpeed directly to _ctx.TransitionSpeed allowing negative values; add validation in OnParametersSet to clamp or reject negatives (e.g., if TransitionSpeed < 0 then set _ctx.TransitionSpeed = 0 or reset TransitionSpeed to 1.0) before assigning to the MotionConfigContext. Update the code that sets _ctx.TransitionSpeed in OnParametersSet to use the validated/clamped value and keep the public property TransitionSpeed unchanged except for validation here.
🧹 Nitpick comments (8)
src/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razor (1)
75-75: 💤 Low valueRemove empty
@codeblock.The empty
@code { }block serves no purpose and can be removed for cleaner code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razor` at line 75, Remove the unnecessary empty Razor code block by deleting the standalone "`@code` { }" declaration (the empty `@code` block in the Keyframes component) so the file no longer contains an unused code region.src/Bmotion/README.md (1)
115-123: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block listing event callbacks should specify a language (e.g.,
text) to satisfy markdown linters and improve documentation consistency.📝 Suggested fix
-``` +```text OnHoverStart / OnHoverEnd OnTapStart / OnTap / OnTapCancel OnFocusStart / OnFocusEnd OnPanStart / OnPan / OnPanEnd (PanInfo) OnDragStart / OnDrag / OnDragEnd OnAnimationStart / OnAnimationComplete OnViewportEnter / OnViewportLeave ```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/README.md` around lines 115 - 123, Update the fenced code block that lists the event callbacks to include a language identifier (e.g., change the opening triple backticks from ``` to ```text) so markdown linters recognize it; ensure the closing triple backticks remain unchanged and no other text is modified.Source: Linters/SAST tools
src/Bmotion/Bit.Bmotion/Models/AnimationTarget.cs (2)
8-21: ⚡ Quick winConsider adding state validation.
The class design implies exactly one of
Props,Variant, orIsDisabledshould be set, but there's no enforcement. A default-constructedAnimationTargetor one created via object initializer could have all properties null/false, creating an invalid state. Consider making the constructor private and only exposing the implicit operators, or adding validation in a constructor.♻️ Proposed fix to prevent invalid states
-public sealed class AnimationTarget +public sealed class AnimationTarget { + private AnimationTarget() { } + /// <summary>Direct set of animation properties.</summary> public AnimationProps? Props { get; private init; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationTarget.cs` around lines 8 - 21, The class allows invalid combinations of Props/Variant/IsDisabled; add a single private ctor that enforces exactly one of Props != null, Variant != null, or IsDisabled == true and throws ArgumentException for invalid combos, then expose public static factory methods (e.g., AnimationTarget.FromProps(AnimationProps props), AnimationTarget.FromVariant(string variant), AnimationTarget.Disabled()) that call the private ctor to create valid instances; update or add any existing implicit operators to use these factories and leave the init-only properties private so instances can only be created via the validated ctor/factories (ensure HasProps and IsVariant remain derived properties).
29-30: 💤 Low valueClarify the semantics of
trueconversion.When
trueis passed, an emptyAnimationPropsinstance is created. Since an emptyAnimationPropshas no properties set, it's unclear what would animate. Consider whether this behavior is intentional or iftrueshould map to a different state (e.g., a default animation or be treated as invalid).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationTarget.cs` around lines 29 - 30, The implicit bool->AnimationTarget conversion currently maps true to an empty AnimationProps which is ambiguous; update the implicit operator AnimationTarget(bool value) to either (preferred) initialize Props with a sensible default animation (e.g., AnimationProps.Default or new AnimationProps { Duration = X, Easing = Y } — pick existing defaults in the project or add a static Default on AnimationProps) so a true value means “enabled with default animation”, or (alternatively) make the operator throw an InvalidOperationException to force callers to supply explicit props; also update the XML comment on the implicit operator and add/adjust tests to reflect the chosen semantics.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs (1)
129-175: Keyframes are not rendered in server-side CSS.The
Keyframesdictionary (which can override single-value properties) is handled inToJsDictionary()but not inToCssStyleString(). This means keyframe-based animations won't have any server-side rendering, which could cause a flash of unstyled content (FOUC) before JavaScript initializes. Consider whether this is acceptable or if keyframes should fallback to their first value in the CSS string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 129 - 175, ToCssStyleString() currently ignores the Keyframes dictionary so server-rendered CSS can miss initial values; update ToCssStyleString() to honor Keyframes as a fallback source for any property not explicitly set (mirroring how ToJsDictionary() uses Keyframes): when computing transforms and simple CSS properties (X/Y/Z, Scale/ScaleX/ScaleY, Rotate/RotateX/RotateY/RotateZ, SkewX/SkewY, Perspective, Opacity, BackgroundColor, Color, BorderColor, Fill, Stroke, Width, Height, BorderRadius, PathLength, and any CssVars keys) check Keyframes for an entry and use its first keyframe value if the explicit property is null/empty, then format that value the same way the method currently formats explicit properties (e.g., produce translate/translate3d/scale/rotate/skew/perspective strings and apply culture-invariant numeric formatting); keep existing behavior when explicit properties exist and ensure PathLength clamping and stroke-dashoffset calculation still use the chosen (explicit or keyframe) value.src/Bmotion/Bit.Bmotion/Models/MotionVariants.cs (1)
12-16: ⚡ Quick winConsider documenting the overwrite behavior or adding validation.
The
Addmethod silently overwrites any existing variant with the same name (case-insensitive). While this might be intentional for a fluent API, it could mask configuration errors where a developer accidentally uses the same variant name twice.Consider either:
- Documenting this behavior in the XML comments
- Adding validation to prevent silent overwrites
📝 Option 1: Document the behavior
+ /// <summary> + /// Adds or updates a variant with the specified name. + /// If a variant with the same name already exists, it will be overwritten. + /// </summary> public MotionVariants Add(string name, AnimationProps props)🛡️ Option 2: Add validation
public MotionVariants Add(string name, AnimationProps props) { + if (_variants.ContainsKey(name)) + throw new ArgumentException($"A variant with the name '{name}' already exists.", nameof(name)); _variants[name] = props; return this; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/MotionVariants.cs` around lines 12 - 16, The Add method in MotionVariants currently overwrites existing entries in the _variants dictionary; change Add(string name, AnimationProps props) to validate first and prevent silent overwrites by checking whether _variants already contains the provided name (respecting the dictionary's case-insensitivity) and throwing an ArgumentException or InvalidOperationException with a clear message if it exists; alternatively (if you prefer keeping overwrite semantics) add/update the XML doc comment on the MotionVariants.Add method to explicitly state that calling Add will replace any existing variant with the same name and consider providing a separate AddOrReplace/Replace method for explicit replacement.src/Bmotion/Bit.Bmotion/Models/PanInfo.cs (1)
10-19: 💤 Low valueConsider removing redundant default initializers.
The
= new()initializers on lines 10, 13, 16, and 19 are redundant. The downstream consumer (Motion.OnPanMove) always constructsPanInfowith explicit property initialization, so these defaults are never used. Removing them simplifies the model and signals that callers must supply values.Alternatively, consider making these properties
init-only to prevent mutation after construction.♻️ Proposed cleanup
- public PointInfo Point { get; set; } = new(); + public PointInfo Point { get; set; } - public PointInfo Delta { get; set; } = new(); + public PointInfo Delta { get; set; } - public PointInfo Offset { get; set; } = new(); + public PointInfo Offset { get; set; } - public PointInfo Velocity { get; set; } = new(); + public PointInfo Velocity { get; set; }Or for immutability:
- public PointInfo Point { get; set; } = new(); + public PointInfo Point { get; init; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/PanInfo.cs` around lines 10 - 19, Remove the redundant "= new()" default initializers from the PanInfo properties and make them init-only to enforce construction-time assignment and immutability: change the Point, Delta, Offset, and Velocity properties to use "get; init;" (referencing the PanInfo class and its Point, Delta, Offset, Velocity properties) so callers like Motion.OnPanMove must supply values at construction and cannot mutate them afterward.src/Bmotion/Bit.Bmotion/Context/PresenceContext.cs (1)
22-27: 💤 Low valueCompletion counter may exceed child count if NotifyExitComplete is called for unregistered children.
The comparison on line 25 uses
>=which guards against over-counting, but ifNotifyExitCompleteis called for a child not in_childrenor after the list has been modified, the event may fire prematurely or multiple times. This is likely fine ifAnimatePresencestrictly controls the registration/notification lifecycle, but consider adding a guard to ensurechildis registered before incrementing.🛡️ Proposed defensive check
internal void NotifyExitComplete(Motion child) { + if (!_children.Contains(child)) return; _completedExits++; if (_completedExits >= _children.Count) AllExitsComplete?.Invoke(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Context/PresenceContext.cs` around lines 22 - 27, NotifyExitComplete currently increments _completedExits unconditionally which can over-count if called for unregistered or duplicate children; update NotifyExitComplete(Motion child) to first verify the child is registered (e.g., if (_children.Contains(child)) { ... }) before incrementing, and guard against double-counting by either removing the child from _children when counted or tracking completed items with a HashSet (e.g., completedChildren.Add(child) and only increment when it was not already present); after the guarded increment, keep the existing check to invoke AllExitsComplete when _completedExits reaches _children.Count (or use completedChildren.Count for the HashSet approach).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7dbd8f9a-d4eb-4f6a-b476-c5d649958c7f
📒 Files selected for processing (69)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/Motion.cssrc/Bmotion/Bit.Bmotion/Components/MotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/MotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/PresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/VariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/ColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/IAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/KeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/SpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/TransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/TweenDriver.cssrc/Bmotion/Bit.Bmotion/Interop/MotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/AnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/AnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/DragOptions.cssrc/Bmotion/Bit.Bmotion/Models/LayoutOptions.cssrc/Bmotion/Bit.Bmotion/Models/MotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/PanInfo.cssrc/Bmotion/Bit.Bmotion/Models/ScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/TransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/ViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/AnimationController.cssrc/Bmotion/Bit.Bmotion/Services/AnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/MotionValue.cssrc/Bmotion/Bit.Bmotion/Services/ScrollTracker.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
src/Bmotion/README.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the JavaScript dependency claim to avoid contradictory docs.
The README currently says “zero JavaScript dependencies” while also documenting an auto-loaded JS bridge. Consider rephrasing to “no manual JS wiring” or “no app-authored JS required.”
Suggested wording patch
-A Blazor-native animation library inspired by [Framer Motion](https://www.framer.com/motion/). Springs, gestures, layout animations, variants, and keyframes - **zero JavaScript dependencies**. All animation math runs in C# via WebAssembly. +A Blazor-native animation library inspired by [Framer Motion](https://www.framer.com/motion/). Springs, gestures, layout animations, variants, and keyframes - **no app-authored JavaScript required**. All animation math runs in C# via WebAssembly. @@ -The browser bridge (`BitBmotion.js`) ships as a static web asset of the package and is -imported automatically the first time an animation runs, so no manual `<script>` tag is -required. +The internal browser bridge (`BitBmotion.js`) ships as a static web asset of the package and is +imported automatically the first time an animation runs, so no manual `<script>` tag is required.Also applies to: 46-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/README.md` at line 3, The README contradicts itself by claiming "zero JavaScript dependencies" while also documenting an auto-loaded JS bridge. Rephrase the dependency claim in the file at lines 3 and also at lines 46-48 (which have similar wording) to clarify that no manual JavaScript wiring or app-authored JavaScript is required, rather than claiming zero JavaScript dependencies altogether. Use language like "no manual JS wiring" or "no app-authored JS required" to accurately reflect that internal JavaScript is auto-loaded but users do not need to write or manage JavaScript code themselves.src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs-113-115 (1)
113-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce the documented
CssVarskey contract.
CssVarsis documented as requiring keys that start with--, but invalid keys are currently serialized as regular CSS properties. This can silently override unrelated styles instead of behaving as custom properties.Suggested fix
if (CssVars != null) foreach (var kv in CssVars) - d[kv.Key] = kv.Value; + { + if (!kv.Key.StartsWith("--", StringComparison.Ordinal)) + throw new ArgumentException("CssVars keys must start with '--'.", nameof(CssVars)); + d[kv.Key] = kv.Value; + } @@ if (CssVars != null) foreach (var kv in CssVars) - sb.Append($"{kv.Key}:{kv.Value};"); + { + if (!kv.Key.StartsWith("--", StringComparison.Ordinal)) + throw new ArgumentException("CssVars keys must start with '--'.", nameof(CssVars)); + sb.Append($"{kv.Key}:{kv.Value};"); + }Also applies to: 172-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs` around lines 113 - 115, The CssVars dictionary is being serialized without validating that keys follow the documented contract of starting with `--` (CSS custom property format). This can cause invalid keys to override unrelated styles. Add validation in both locations where CssVars are processed (lines 113-115 and lines 172-174) to check that each key in CssVars starts with `--` before adding it to the dictionary d. Either skip keys that don't meet this requirement or throw an appropriate exception to enforce the contract and prevent silent style overrides.src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs-20-24 (1)
20-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
Transitiondefault documentation with the actual declaration.The comment says this defaults to a snappy spring, but this property is declared nullable with no in-type default initializer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs` around lines 20 - 24, The XML documentation comment for the Transition property in LayoutOptions claims it "Defaults to a snappy spring," but the property is declared as TransitionConfig? (nullable) with no initializer, meaning it has no actual default value. Update the documentation comment to accurately reflect that this property is nullable and either describe the actual default behavior when null, or remove the claim about defaulting to a snappy spring. Alternatively, if a snappy spring should truly be the default, add an initializer to provide that default value.src/Bmotion/Bit.Bmotion/Models/DragOptions.cs-21-21 (1)
21-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp
Elasticto the documented 0..1 range before serialization.
Elasticis documented as bounded, butToJsObject()currently forwards any value as-is.Suggested fix
- ["dragElastic"] = Elastic, + ["dragElastic"] = Math.Clamp(Elastic, 0d, 1d),Also applies to: 53-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/DragOptions.cs` at line 21, The Elastic property is documented as bounded to a 0..1 range, but the ToJsObject() method does not enforce this constraint before serialization. Modify the ToJsObject() method to clamp the Elastic property value to the 0..1 range using Math.Max and Math.Min before including it in the serialized output. Apply the same clamping logic to the other property mentioned at line 53 that has similar bounded documentation.src/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cs-55-61 (1)
55-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd upfront selector validation in
AnimateAsync(string selector, ...).Line 60 forwards selector directly; null/whitespace should fail fast with a clear argument exception.
Proposed fix
public async ValueTask<AnimationControls> AnimateAsync( string selector, AnimationProps keyframes, TransitionConfig? transition = null) { + if (string.IsNullOrWhiteSpace(selector)) + throw new ArgumentException("Selector must be non-empty.", nameof(selector)); + var ids = await _interop.ResolveOrRegisterBySelectorAsync(selector); return StartAnimations(ids, keyframes, transition); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cs` around lines 55 - 61, The AnimateAsync method accepts a selector string parameter but passes it directly to _interop.ResolveOrRegisterBySelectorAsync without validation. Add upfront validation in the AnimateAsync method to check if the selector parameter is null or empty/whitespace, and throw an ArgumentException with a descriptive message if validation fails. This should occur before calling ResolveOrRegisterBySelectorAsync to fail fast with clear error information.src/Bmotion/Bit.Bmotion/Services/AnimationController.cs-20-20 (1)
20-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
elementIdinBindTobefore storing it.Line 20 accepts empty/whitespace IDs, which can surface later as runtime failures in engine/interop calls.
Proposed fix
- public void BindTo(string elementId) => _elementId = elementId; + public void BindTo(string elementId) + { + if (string.IsNullOrWhiteSpace(elementId)) + throw new ArgumentException("Element id must be non-empty.", nameof(elementId)); + _elementId = elementId; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/AnimationController.cs` at line 20, The BindTo method in the AnimationController class does not validate the elementId parameter before storing it, allowing empty or whitespace values to be assigned to the _elementId field. This leads to runtime failures when the invalid ID is used later in engine or interop calls. Add validation logic at the start of the BindTo method to check if elementId is null, empty, or whitespace, and throw an appropriate exception (such as ArgumentException or ArgumentNullException) with a descriptive message if validation fails. Only assign the value to _elementId if validation passes.src/Bmotion/Bit.Bmotion/Engine/TransformComposer.cs-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid emitting identity
scale(1)inBuild.Line 41-43 currently adds
scale(...)even when the value is1, which contradicts the “empty when identity” contract and causes unnecessary style churn.Proposed fix
- if (t.TryGetValue("scale", out double scale)) - parts.Add($"scale({scale})"); + if (t.TryGetValue("scale", out double scale) && scale != 1) + parts.Add($"scale({scale})");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/TransformComposer.cs` around lines 41 - 43, The TransformComposer Build method is currently adding scale(1) to the parts list even when the scale value is 1, which contradicts the contract to return empty content for identity values. Modify the condition in the if statement that checks t.TryGetValue("scale", out double scale) to additionally verify that scale is not equal to 1 before calling parts.Add. This prevents unnecessary style output when the scale transform is the identity value.
🧹 Nitpick comments (1)
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs (1)
44-48: ⚡ Quick winAdd an identity test case for uniform scale.
Build_AllIdentityValues_ReturnsEmptyis a great guard, but it doesn’t include["scale"] = 1. Adding that case will better lock the identity contract.Suggested test addition
[TestMethod] public void Build_AllIdentityValues_ReturnsEmpty() { var t = new Dictionary<string, double> { ["x"] = 0, ["y"] = 0, ["rotate"] = 0 }; Assert.AreEqual(string.Empty, TransformComposer.Build(t)); } + +[TestMethod] +public void Build_UniformScaleIdentity_ReturnsEmpty() +{ + var t = new Dictionary<string, double> { ["scale"] = 1.0 }; + Assert.AreEqual(string.Empty, TransformComposer.Build(t)); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs` around lines 44 - 48, The test method `Build_AllIdentityValues_ReturnsEmpty` currently only tests identity values for x, y, and rotate, but omits the scale property. Add the scale identity value (scale = 1) to the Dictionary initialization in this test method to ensure the identity contract is fully validated across all transform properties. This will strengthen the test coverage by verifying that the TransformComposer.Build method correctly returns an empty string when all properties including scale are at their identity values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 35-49: The AnimatePresence component with IsPresent="true" is
currently placed inside the foreach loop wrapping each individual item. When
RemoveItem is called, the item is removed from the _items collection before
IsPresent can transition to false, preventing the exit animation from playing.
Move the AnimatePresence wrapper outside the foreach loop to wrap the entire
collection, and ensure each Motion component has a unique `@key` attribute.
Alternatively, maintain a presence flag for each item in your data model and set
it to false before removing the item from the collection, allowing the exit
animation to complete before the item is dropped from the loop.
In `@src/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cs`:
- Around line 39-41: The initialization of _prevIsPresent and _shouldRender both
start as true, which incorrectly treats the first parameter set with
IsPresent=false as an exit transition. Initialize _prevIsPresent to false
instead of true to reflect the actual initial state where nothing is present, so
that an initial IsPresent=false parameter value is not treated as a leave
transition. This ensures the presence logic only treats actual state changes
from present to not-present as exit transitions, not the initial parameter set.
In `@src/Bmotion/Bit.Bmotion/Components/Motion.cs`:
- Around line 132-139: The Motion component creates a fresh VariantContext when
Variants are defined, but it only sets the ActiveVariant based on the local
Animate property without preserving any inherited active variant from an
ancestor component. When setting _ownVariantCtx.ActiveVariant, check if there's
an inherited active variant available (from a parent VariantContext) and use
that as a fallback when the current component doesn't explicitly override it
with its own Animate.Variant. This ensures that nested variant trees properly
inherit and propagate the active label through the component hierarchy, allowing
stagger and variant propagation to work correctly at all levels.
- Around line 379-385: The OnPointerUp method in Motion.cs deactivates the
gesture layer when WhileTap is active, but fails to invoke the OnTapCancel
callback when the user releases outside the element (isInsideElement is false).
This is the most common cancellation scenario. After the gesture layer
deactivation, add a check for when isInsideElement is false and invoke
OnTapCancel.InvokeAsync() to notify subscribers of the cancelled tap gesture.
In `@src/Bmotion/Bit.Bmotion/Context/PresenceContext.cs`:
- Around line 10-16: The _children field uses a List<Motion> which allows
duplicate registrations, but the completion tracking uses a HashSet that won't
account for duplicates, causing AllExitsComplete to fail when a child is
registered multiple times. Change the _children field from List<Motion> to
HashSet<Motion> to prevent duplicate registrations, and update the Register and
Unregister methods to work with the HashSet's Add and Remove operations
respectively. This ensures the count of children matches the count of completed
children for proper exit animation tracking.
In `@src/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cs`:
- Around line 199-255: The issue is that when momentum is true but both velocity
components are below the inertia threshold (0.5), the constraint snap-back logic
is skipped entirely because it lives under the else if (constraints != null)
branch. This allows the element to remain outside its bounds. Change the else if
to just if for the constraint recovery block so that snap-back logic executes
independently of the momentum state. This ensures that whenever constraints are
defined and the element is out of bounds (regardless of whether momentum
animations were triggered), the element will snap back to valid bounds using the
snapTransition.
- Around line 309-315: The EnsureLoopRunningAsync method sets _loopRunning to
true before calling the asynchronous StartRafLoopAsync operation, which means if
that call throws an exception, _loopRunning remains true while the loop never
actually started. This causes all future calls to return early without retrying.
Move the _loopRunning = true assignment to after the await
_interop.StartRafLoopAsync(_dotnet) call completes successfully, so the flag
only gets set if the startup succeeds.
In `@src/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cs`:
- Around line 16-20: The string interpolation format for the alpha value in the
Lerp method uses the current culture, which causes CSS color strings to have
commas as decimal separators in cultures with comma decimals (e.g.,
rgba(255,128,64,0,5) instead of rgba(255,128,64,0.5)), making the CSS invalid.
Replace the current culture-dependent formatting with invariant culture
formatting for the alpha value in the return statement at lines 16-20.
Additionally, apply the same invariant culture fix to any double.Parse calls and
string interpolations elsewhere in the file that handle color component values,
particularly at lines 57-78, to ensure all CSS color numbers consistently use
dots as decimal separators regardless of the user's system culture.
In `@src/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cs`:
- Around line 46-59: The ToCssString method is missing easing mode cases that
are supported by the Get(...) method, specifically Circ*, Back*, and Anticipate
modes. When Motion.PlayFlipAsync() uses this method for layout transitions,
these unsupported modes incorrectly fall through to the default "ease" case,
causing inconsistent behavior. Add switch cases in the ToCssString method for
all easing modes that Get(...) supports, either by mapping them to their CSS
cubic-bezier equivalents (for Circ* modes) or providing appropriate fallback
values for modes without direct CSS equivalents (Back* and Anticipate modes).
In `@src/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cs`:
- Around line 222-226: The ActivateGestureLayer method unconditionally calls
AnimateTo() without checking gesture priority, allowing lower-priority gestures
to override higher-priority ones that are already active. After storing the new
gesture layer in _gestureLayers[gesture], add a priority check before calling
AnimateTo() to ensure the newly activated gesture has sufficient priority to
animate to those values. Only proceed with the AnimateTo() call if the new
gesture's priority is higher than any other currently active gesture, mirroring
the priority logic already applied in DeactivateGestureLayer().
- Around line 144-151: The keyframe dispatch logic at lines 144-151 treats all
string arrays as color animations without verifying the property is actually a
color property, and the helper methods at lines 343-399 are too restrictive. To
fix: at lines 144-151, reorder or add property-type checks so that
CreateColorKeyframesDriver is only called when IsColorProp(key) returns true AND
the value is a string array; at lines 343-399, widen TryGetDoubleArray and
TryGetStringArray to accept IEnumerable<T> instead of just array types,
supporting List<double>, List<string>, int[], float[], and other common C#
numeric and string collection types so that inputs like new[] { 0, 100 },
List<double>, width = ["10px", "20px"] are properly recognized and dispatched to
the correct driver.
- Around line 157-183: The SetInstant method updates the backing dictionaries
but does not cancel any existing drivers/animators for those properties, which
will overwrite the instant values on the next tick. Before updating the
Transforms, StringValues, or NumericValues dictionaries in SetInstant, cancel
any active drivers for each property key being updated to ensure the instant
values are authoritative and not immediately overwritten by ongoing animations.
- Around line 185-210: The CancelProp method removes individual animation
drivers but fails to complete the _completionSource, unlike CancelAll(). When
Cancel(string[] properties) is called with specific property names, it removes
drivers one by one via CancelProp, but if those are the only active animations,
the _completionSource never gets resolved, causing AnimateToAwaitAsync() to hang
indefinitely. Modify the CancelProp method to check if _activeAnims is empty
after removing a driver, and if so, complete the _completionSource by calling
TrySetResult() and setting _completionSource to null, matching the behavior in
CancelAll().
In `@src/Bmotion/Bit.Bmotion/Engine/KeyframesDriver.cs`:
- Around line 61-63: When the repeat type is Mirror or Reverse, the code
reverses the _curFrames array but fails to reverse the corresponding Times
array, causing timing misalignment on non-uniform timelines. At the anchor site
(lines 61-63) where Array.Reverse is called on _curFrames within the numeric
driver's repeat logic, also reverse the Times array to mirror the timeline.
Apply the same fix at the sibling site (lines 147-149) in the color driver's
repeat logic, ensuring both the frame order and the timeline order are reversed
together to maintain correct segment timing.
In `@src/Bmotion/Bit.Bmotion/Models/TransitionConfig.cs`:
- Line 23: The EaseCubicBezier property at line 23 allows any double array
without validating the strict 4-item contract required for animation behavior.
Add validation logic to ensure that whenever EaseCubicBezier is set (either
through the property setter or during deserialization at line 127), it is either
null or contains exactly 4 finite numeric values. Implement this validation to
reject arrays of incorrect length or containing NaN/Infinity values before the
property is assigned or serialized. This prevents invalid animation
configuration from leaking into runtime.
In `@src/Bmotion/Bit.Bmotion/Models/ViewportOptions.cs`:
- Around line 34-40: The threshold switch expression in ViewportOptions.cs
treats the Amount property case-sensitively for the "some" and "all" cases, and
does not validate that parsed double values are finite, allowing invalid
thresholds like NaN or infinity to propagate. Convert Amount to lowercase before
the switch statement to ensure case-insensitive matching for "some" and "all",
and add a check using double.IsFinite() in the default case to validate the
parsed numeric value is finite before clamping and returning it; non-finite
values should default to 0.0.
In `@src/Bmotion/Bit.Bmotion/Services/AnimationControls.cs`:
- Around line 37-41: The Complete() method is not implementing its documented
contract of snapping animations to target end values. Instead, it calls
_engine.Stop(id, null) which is identical to the Stop() method behavior. Replace
the Stop() call in the Complete() method body with the appropriate engine method
call that actually implements the completion behavior (snapping to target end
values), rather than just halting the animation.
In `@src/Bmotion/Bit.Bmotion/Services/MotionValue.cs`:
- Around line 32-37: The subscriber invocation itself needs error handling, not
just the returned task. In SetSync, SetAsync, and the third affected location,
wrap the `sub(value)` call in a try-catch block to handle cases where a Func<T,
Task> subscriber throws synchronously before returning a task. If a subscriber
throws, catch the exception, log it appropriately, and continue invoking
remaining subscribers instead of allowing the exception to propagate and skip
the rest of the subscription chain.
- Around line 72-108: Both Transform method overloads (the generic
Transform<TOut> method at lines 72-76 and the Transform method with
inputRange/outputRange parameters at lines 81-108, as well as the related
overloads that apply similar patterns around lines 111-118) are not disposing of
the IDisposable returned by the Subscribe() call. This leaves the upstream
subscription active even after the derived MotionValue is disposed, causing
resource leaks. Store the result of each Subscribe() call and ensure it is
properly disposed when the derived MotionValue is disposed. You can accomplish
this by passing the subscription to the derived MotionValue instance so it can
manage the subscription lifecycle, or by returning a composite disposable that
includes both the derived value and the subscription, allowing the caller to
manage both resources together.
In `@src/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.js`:
- Around line 206-255: The _attachPan function doesn't track whether a pointer
is currently pressed, allowing stale startX/startY coordinates from previous
gestures to trigger pan detection during subsequent hover movements. Add a flag
variable to track the active pointer state (set to true in the onDown handler,
false in the onUp handler), and in the onMove handler, only perform pan
detection and invoke pan callbacks when this flag indicates a pointer is
actually down.
---
Minor comments:
In `@src/Bmotion/Bit.Bmotion/Engine/TransformComposer.cs`:
- Around line 41-43: The TransformComposer Build method is currently adding
scale(1) to the parts list even when the scale value is 1, which contradicts the
contract to return empty content for identity values. Modify the condition in
the if statement that checks t.TryGetValue("scale", out double scale) to
additionally verify that scale is not equal to 1 before calling parts.Add. This
prevents unnecessary style output when the scale transform is the identity
value.
In `@src/Bmotion/Bit.Bmotion/Models/AnimationProps.cs`:
- Around line 113-115: The CssVars dictionary is being serialized without
validating that keys follow the documented contract of starting with `--` (CSS
custom property format). This can cause invalid keys to override unrelated
styles. Add validation in both locations where CssVars are processed (lines
113-115 and lines 172-174) to check that each key in CssVars starts with `--`
before adding it to the dictionary d. Either skip keys that don't meet this
requirement or throw an appropriate exception to enforce the contract and
prevent silent style overrides.
In `@src/Bmotion/Bit.Bmotion/Models/DragOptions.cs`:
- Line 21: The Elastic property is documented as bounded to a 0..1 range, but
the ToJsObject() method does not enforce this constraint before serialization.
Modify the ToJsObject() method to clamp the Elastic property value to the 0..1
range using Math.Max and Math.Min before including it in the serialized output.
Apply the same clamping logic to the other property mentioned at line 53 that
has similar bounded documentation.
In `@src/Bmotion/Bit.Bmotion/Models/LayoutOptions.cs`:
- Around line 20-24: The XML documentation comment for the Transition property
in LayoutOptions claims it "Defaults to a snappy spring," but the property is
declared as TransitionConfig? (nullable) with no initializer, meaning it has no
actual default value. Update the documentation comment to accurately reflect
that this property is nullable and either describe the actual default behavior
when null, or remove the claim about defaulting to a snappy spring.
Alternatively, if a snappy spring should truly be the default, add an
initializer to provide that default value.
In `@src/Bmotion/Bit.Bmotion/Services/AnimationController.cs`:
- Line 20: The BindTo method in the AnimationController class does not validate
the elementId parameter before storing it, allowing empty or whitespace values
to be assigned to the _elementId field. This leads to runtime failures when the
invalid ID is used later in engine or interop calls. Add validation logic at the
start of the BindTo method to check if elementId is null, empty, or whitespace,
and throw an appropriate exception (such as ArgumentException or
ArgumentNullException) with a descriptive message if validation fails. Only
assign the value to _elementId if validation passes.
In `@src/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cs`:
- Around line 55-61: The AnimateAsync method accepts a selector string parameter
but passes it directly to _interop.ResolveOrRegisterBySelectorAsync without
validation. Add upfront validation in the AnimateAsync method to check if the
selector parameter is null or empty/whitespace, and throw an ArgumentException
with a descriptive message if validation fails. This should occur before calling
ResolveOrRegisterBySelectorAsync to fail fast with clear error information.
In `@src/Bmotion/README.md`:
- Line 3: The README contradicts itself by claiming "zero JavaScript
dependencies" while also documenting an auto-loaded JS bridge. Rephrase the
dependency claim in the file at lines 3 and also at lines 46-48 (which have
similar wording) to clarify that no manual JavaScript wiring or app-authored
JavaScript is required, rather than claiming zero JavaScript dependencies
altogether. Use language like "no manual JS wiring" or "no app-authored JS
required" to accurately reflect that internal JavaScript is auto-loaded but
users do not need to write or manage JavaScript code themselves.
---
Nitpick comments:
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cs`:
- Around line 44-48: The test method `Build_AllIdentityValues_ReturnsEmpty`
currently only tests identity values for x, y, and rotate, but omits the scale
property. Add the scale identity value (scale = 1) to the Dictionary
initialization in this test method to ensure the identity contract is fully
validated across all transform properties. This will strengthen the test
coverage by verifying that the TransformComposer.Build method correctly returns
an empty string when all properties including scale are at their identity
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 389d34b5-0098-418e-92a9-916633506126
📒 Files selected for processing (69)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/AnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/Motion.cssrc/Bmotion/Bit.Bmotion/Components/MotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/MotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/PresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/VariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/AnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/ColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/ColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/EasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/ElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/IAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Engine/InertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/KeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/SpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/TransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/TweenDriver.cssrc/Bmotion/Bit.Bmotion/Interop/MotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/AnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/AnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/DragOptions.cssrc/Bmotion/Bit.Bmotion/Models/LayoutOptions.cssrc/Bmotion/Bit.Bmotion/Models/MotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/PanInfo.cssrc/Bmotion/Bit.Bmotion/Models/ScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/TransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/ViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/AnimationController.cssrc/Bmotion/Bit.Bmotion/Services/AnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/MotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/MotionValue.cssrc/Bmotion/Bit.Bmotion/Services/ScrollTracker.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/BitBmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (2)
src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs (1)
169-176:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPath drawing output is still using the broken dash math.
Line 174/175 and Line 234/235 still emit normalized dash values that don’t reliably produce progressive SVG path drawing. This is the same unresolved issue previously reported, now duplicated in both CSS emitters (
ToCssStyleStringandToCssStyleDictionary).Also applies to: 229-236
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs` around lines 169 - 176, The path drawing output contains broken dash math that does not reliably produce progressive SVG path drawing, and this issue is duplicated across two CSS emitter methods. In the ToCssStyleString method (around lines 169-176 with stroke-dasharray and stroke-dashoffset calculations), fix the normalized dash value calculations used in the stroke-dasharray and stroke-dashoffset assignments to use correct math for progressive path drawing. Then apply the same corrected formula to the duplicate code in ToCssStyleDictionary method (around lines 229-236) which has the identical stroke-dasharray and stroke-dashoffset calculations. Both methods must use consistent and correct dash math to ensure reliable progressive SVG path drawing behavior.src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs (1)
20-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the documented cubic-bezier invariant at assignment time.
Line 20 documents a strict 4-value bezier contract, but Line 22 currently accepts any array and Line 129 clones it unchanged. Please validate
EaseCubicBezieras eithernullor exactly 4 finite values to prevent invalid transition configs from reaching runtime.Proposed minimal fix
- public double[]? EaseCubicBezier { get; set; } + private double[]? easeCubicBezier; + public double[]? EaseCubicBezier + { + get => easeCubicBezier; + set + { + if (value is not null && (value.Length != 4 || value.Any(v => !double.IsFinite(v)))) + throw new ArgumentException("EaseCubicBezier must contain exactly 4 finite values [x1, y1, x2, y2]."); + easeCubicBezier = value; + } + }Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs` around lines 20 - 23, The EaseCubicBezier property currently lacks validation despite its documentation requiring exactly 4 finite values in the format [x1, y1, x2, y2]. Add validation logic to the EaseCubicBezier property setter (around line 22) to ensure the assigned value is either null or an array with exactly 4 finite numeric values; reject any other input. Additionally, ensure the cloning operation at line 129 also validates the array to maintain the invariant, so invalid configs cannot persist through serialization or copying operations.
🟡 Minor comments (8)
src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor-47-48 (1)
47-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an accessible name to the remove button.
At Line 47–48, the icon-only
✕button has no explicit accessible label, so screen readers may announce it ambiguously. Add anaria-label(and mirror the same change in the_listCodesnippet around Line 170–171 for consistency).Suggested fix
- <button style="background:none;border:none;color:`#ff4785`;cursor:pointer;font-size:1.1rem" - `@onclick`="@(() => RemoveItem(item.Id))">✕</button> + <button style="background:none;border:none;color:`#ff4785`;cursor:pointer;font-size:1.1rem" + aria-label="@($"Remove {item.Label}")" + title="@($"Remove {item.Label}")" + `@onclick`="@(() => RemoveItem(item.Id))">✕</button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor` around lines 47 - 48, The remove button displaying the ✕ icon lacks an accessible label for screen readers. Add an aria-label attribute to the button element in the RemoveItem button (around lines 47-48) with a descriptive label such as "Remove item". Apply the same aria-label attribute to the corresponding remove button in the _listCode snippet (around lines 170-171) to maintain consistency across both locations.src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor-35-35 (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
TransitionSpeedagainst non-finite values.
TransitionSpeed < 0 ? 0 : TransitionSpeedstill allowsNaN/Infinity, which can break downstream duration calculations.Suggested fix
- _ctx.TransitionSpeed = TransitionSpeed < 0 ? 0 : TransitionSpeed; + _ctx.TransitionSpeed = double.IsFinite(TransitionSpeed) + ? Math.Max(0, TransitionSpeed) + : 1.0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor` at line 35, The guard for TransitionSpeed in the BmotionConfig.razor component only checks if the value is negative but allows non-finite values like NaN and Infinity to pass through, which can break downstream duration calculations. Modify the assignment of _ctx.TransitionSpeed to validate not only that it is non-negative but also that it is a finite number by using a method like double.IsFinite() in addition to the negative check, defaulting to 0 for any invalid values.src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs-68-76 (1)
68-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate subscriber callbacks for null.
Adding null callbacks currently defers failures to notification time; reject invalid registrations immediately.
Suggested fix
public IDisposable Subscribe(Func<T, Task> callback) { + ArgumentNullException.ThrowIfNull(callback); _subscribers.Add(callback); return new Subscription(() => _subscribers.Remove(callback)); } public IDisposable Subscribe(Action<T> callback) - => Subscribe(v => { callback(v); return Task.CompletedTask; }); + { + ArgumentNullException.ThrowIfNull(callback); + return Subscribe(v => { callback(v); return Task.CompletedTask; }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs` around lines 68 - 76, Both Subscribe method overloads accept null callbacks without validation, allowing the failure to be deferred until notification time instead of rejecting the invalid registration immediately. Add null validation at the start of each Subscribe overload to throw ArgumentNullException if the callback parameter is null. Apply this check to the async version Subscribe(Func<T, Task> callback) and the synchronous convenience overload Subscribe(Action<T> callback) before the callback is used or added to the collection.src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs-47-47 (1)
47-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject null
onChangeat subscription time.Failing fast here avoids deferred null callback failures during scroll events.
Suggested fix
public async Task ObserveAsync(string? containerId, Func<BmotionScrollInfo, Task> onChange) { + ArgumentNullException.ThrowIfNull(onChange); ObjectDisposedException.ThrowIf(_disposed, this);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs` at line 47, Add a null guard at the start of the ObserveAsync method to validate that the onChange parameter is not null. If onChange is null, throw an ArgumentNullException with an appropriate parameter name. This prevents deferred failures when the callback is invoked during scroll events by catching the issue early at subscription time.src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs-7-8 (1)
7-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winXML docs contradict the actual API contract.
Line 7 says
Null = both axes, butAxisis a non-nullableBmotionDragAxis(Line 8) and defaults toBoth. Please align the comment to avoid misleading IntelliSense/API usage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs` around lines 7 - 8, The XML documentation on the Axis property in BmotionDragOptions contradicts the actual implementation. The property is non-nullable with a default value of BmotionDragAxis.Both, not nullable. Update the summary documentation for the Axis property to remove the reference to "Null = both axes" and instead clarify that the property defaults to Both when not explicitly set, reflecting the actual non-nullable contract of the property.src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs-17-22 (1)
17-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject negative
TransitionSpeedvalues.Line 21 accepts any
double; negative speeds break the documented timing contract and can silently collapse animations instead of scaling them.Suggested fix
-public double TransitionSpeed { get; set; } = 1.0; +private double _transitionSpeed = 1.0; +public double TransitionSpeed +{ + get => _transitionSpeed; + set => _transitionSpeed = value < 0 + ? throw new ArgumentOutOfRangeException(nameof(value), "TransitionSpeed must be >= 0.") + : value; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs` around lines 17 - 22, The TransitionSpeed property currently accepts any double value without validation, but negative speeds are invalid according to the documented contract and will cause animations to malfunction. Add validation to the TransitionSpeed property setter to reject negative values by throwing an appropriate exception (such as ArgumentException or ArgumentOutOfRangeException) when a negative value is assigned, ensuring that only non-negative values are accepted.src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs-494-497 (1)
494-497:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Cast<string>()may throw if not all elements are strings.The check
oa[0] is stringonly verifies the first element. If subsequent elements are non-strings,Cast<string>()will throwInvalidCastException.Proposed fix
if (value is object[] oa && oa.Length > 0 && oa[0] is string) { - result = oa.Cast<string>().ToArray(); + if (oa.All(x => x is string)) + { + result = oa.Cast<string>().ToArray(); + return true; + } - return true; + return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs` around lines 494 - 497, The current code validates only the first element with `oa[0] is string`, but `Cast<string>()` will throw `InvalidCastException` if any subsequent element is not a string. Fix this by adding a validation check that ensures ALL elements in the `oa` array are strings before calling `Cast<string>()`. Use `oa.All(x => x is string)` in addition to (or instead of) the existing `oa[0] is string` check to validate the entire array before the cast operation.src/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cs-17-19 (1)
17-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the precision contract mismatch in
Num(double).Line 17 says “full round-trip precision,” but Line 19 formats with
"G6"(lossy). Either change the format to a round-trip one ("R"/"G17") or update the XML summary to match the intended compact formatting.💡 Minimal fix (doc-aligned)
- /// <summary>Formats a double as an invariant-culture string with full round-trip precision.</summary> + /// <summary>Formats a double as a compact invariant-culture string (6 significant digits).</summary> public static string Num(double value) => value.ToString("G6", CultureInfo.InvariantCulture);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cs` around lines 17 - 19, The `Num(double)` method has a contract mismatch between its XML documentation and implementation. The documentation states it provides "full round-trip precision," but the implementation uses `"G6"` format which is lossy and not full round-trip. Either replace the format string `"G6"` with `"R"` (or `"G17"`) to achieve actual round-trip precision, or update the XML summary comment to accurately describe the lossy `"G6"` compact formatting behavior to match the intended implementation.
🧹 Nitpick comments (1)
src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor (1)
53-54: UseOnDragStartto avoid redundant per-frame status updates during drag.
OnDragfires continuously while dragging and sets_dragInfoto the same"Dragging…"value each frame, causing unnecessary renders. Move this status update toOnDragStart, which fires only once at drag initiation, keepingOnDragEndfor the"Released"state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor` around lines 53 - 54, The OnDrag event handler fires continuously during drag operations, causing redundant per-frame updates to _dragInfo with the "Dragging…" status and unnecessary renders. Add an OnDragStart event binding to the component and create a HandleDragStart method that sets _dragInfo to "Dragging…" once at drag initiation. Remove or refactor the HandleDrag method to eliminate the redundant status update, keeping only the necessary drag tracking logic. Keep HandleDragEnd for setting the "Released" state when the drag completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razor`:
- Around line 40-46: The CopyAsync method uses InvokeVoidAsync which discards
the boolean result returned by the bmCopyToClipboard JavaScript function,
causing _copied to be set to true even when the copy operation fails. Change the
InvokeVoidAsync call to InvokeAsync<bool> to capture the actual boolean result
from bmCopyToClipboard, then only set _copied = true when the returned value is
actually true, not unconditionally.
In `@src/Bmotion/Bit.Bmotion/Components/Bmotion.cs`:
- Around line 130-135: The _ownVariantCtx.InitialVariant assignment is resetting
to null when there is no local Initial variant, but it should inherit from the
parent context instead. Modify the InitialVariant assignment to follow the same
fallback pattern as ActiveVariant: when Initial?.IsVariant is false, use
VariantCtx?.InitialVariant as the fallback value instead of null. This ensures
that descendants preserve inherited initial variant labels when the current node
defines its own Variants without defining a local Initial variant.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cs`:
- Around line 45-48: The field _shouldRender is initialized to true, but when
IsPresent is false on the initial render, the OnParametersSet method does not
set _shouldRender to false for the false→false transition case, causing children
to mount despite being initially absent. Change the initialization of
_shouldRender from true to false so that the component does not render when
IsPresent is false on first render, then ensure the transition logic in
OnParametersSet (around lines 60-85) properly handles all state transitions to
update _shouldRender accordingly.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs`:
- Around line 33-35: Both keyframe driver constructors lack input validation
which can cause crashes during animation. In
src/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cs (lines 33-35),
add validation to ensure frames.Length is at least 2 and that the Times array
length matches frames.Length, throwing an appropriate exception if either
condition fails. Apply the identical validation logic to
src/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cs (lines 33-36) to
maintain consistent safety guarantees across both implementations and prevent
interpolation indexing failures.
- Around line 34-35: Both BmotionNumericKeyframesDriver.cs (lines 34-35) and
BmotionColorKeyframesDriver.cs (lines 34-35) are storing direct references to
the config.Times array and then mutating it during mirror/reverse operations,
which modifies the caller's original data. Clone the config.Times array
immediately after the null-coalescing assignment (before any subsequent
mutations or operations) in both files by calling .ToArray() or .Clone() on the
assigned value, so that _times holds an independent copy that can be safely
modified without affecting the caller's original configuration object.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cs`:
- Around line 49-50: The rotation assignment at line 49 in the
BmotionTransformComposer.cs file incorrectly prioritizes a zero value from
"rotateZ" over a non-zero "rotate" value. Modify the logic for assigning the rz
variable so that it only uses the "rotateZ" value when it is non-zero (excluding
the identity case), and falls back to "rotate" when "rotateZ" is absent or is
zero. This ensures that non-zero rotation values from either property are
preserved in the output.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cs`:
- Around line 36-38: All four animation driver implementations incorrectly apply
a final value when cancelled instead of freezing at the current position,
violating the documented interface contract. In the Tick method of each driver
class, remove the _apply() call from the cancellation check: in
BmotionTweenDriver.cs (lines 36-38) change the condition to just `if
(_cancelled) return true;` by removing `_apply(_to);`, in
BmotionColorTweenDriver.cs (lines 36-38) do the same by removing `_apply(_to);`,
in BmotionInertiaDriver.cs (lines 41-43) change to `if (_cancelled) return
true;` by removing `_apply(_projected);`, and in BmotionSpringDriver.cs (lines
67-69) change to `if (_cancelled) return true;` by removing `_apply(_target);`.
This ensures animations freeze at their current intermediate position when
cancelled rather than jumping to the target value.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs`:
- Around line 51-54: The `keyframes` parameter in the `AnimateAsync` method
signature (at lines 51-54) is not being validated before use, and when
`keyframes.ToJsDictionary()` is called, it can throw if null is passed. Add an
`ArgumentNullException` validation at the beginning of the `AnimateAsync` method
body to fail fast at the API boundary. Apply the same null validation for the
`keyframes` parameter at the other affected locations (lines 74-77 and line 90)
where similar keyframes parameter usage occurs without validation.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs`:
- Around line 20-26: The BindTo method registers new element IDs via
_engine.RegisterElement but never unregisters the previously bound element ID
before rebinding. When BindTo is called multiple times with different IDs, the
old registrations accumulate in the engine causing a leak. Before registering
the new elementId, check if _elementId is already set to a non-empty value and
unregister it via _engine.UnregisterElement. Additionally, consider implementing
IDisposable on the BmotionAnimationController class to unregister the element
when the controller's lifetime ends.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 67-75: The OnScroll method does not handle exceptions that may be
thrown by the _onScroll callback, which can destabilize the interop flow and
host circuit/runtime. Wrap the await _onScroll(info) invocation in a try-catch
block to catch and handle any exceptions thrown by the callback. Log or handle
the exception appropriately to prevent it from propagating and faulting the
JS-invokable callback flow.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs`:
- Around line 96-108: The Transform method can be called with non-numeric
generic types like Guid or DateTime, and Convert.ToDouble(v) in the nested Map
function will throw a runtime exception with an unclear error message. Add an
explicit check early in the Transform method to verify that the generic type T
is numeric (such as int, double, float, decimal, etc.), and if not, throw an
ArgumentException with a clear message indicating that Transform only works with
numeric types. This ensures the error is caught and reported clearly rather than
bubbling up from Convert.ToDouble with a cryptic message.
In `@src/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.js`:
- Around line 36-48: The `_tick` function is vulnerable to exceptions from the
`invokeMethod('ComputeFrame', timestamp)` call, which can skip the
`requestAnimationFrame(_tick)` scheduling on line 47 and stall animations
indefinitely. Wrap the `invokeMethod` call and the subsequent style application
logic in a try-catch block to safely handle any exceptions, ensuring that
`requestAnimationFrame(_tick)` is always executed after the try-catch block
completes, regardless of whether an exception occurred. This guarantees the
animation loop continues even after transient frame failures.
- Around line 370-378: The observeViewport function can create duplicate
IntersectionObserver subscriptions for the same element when called multiple
times with different options, as line 377 reuses or creates a new observer
without first removing the element from any previously active observer. Before
calling _getVpObserver(margin, threshold).observe(el), you need to check if the
element is already being observed and unobserve it from its previous observer to
prevent stacking subscriptions. This ensures that only one observer subscription
exists per element and that the once behavior works correctly across option
changes.
In `@src/Bmotion/README.md`:
- Around line 54-62: Update all code examples in the README to use the actual
prefixed API names from the implementation. Replace all unprefixed component and
class names with their Bmotion-prefixed equivalents: change <Motion> to
<Bmotion>, <AnimatePresence> to <BmotionAnimatePresence>, <MotionConfig> to
<BmotionConfig>, AnimationProps to BmotionAnimationProps, TransitionConfig to
BmotionTransitionConfig, ViewportOptions to BmotionViewportOptions, Easing to
BmotionEasing, and RepeatType to BmotionRepeatType. Ensure all examples
throughout the README documentation use the correct prefixed names so users can
copy and use the code without compilation errors.
---
Minor comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razor`:
- Around line 47-48: The remove button displaying the ✕ icon lacks an accessible
label for screen readers. Add an aria-label attribute to the button element in
the RemoveItem button (around lines 47-48) with a descriptive label such as
"Remove item". Apply the same aria-label attribute to the corresponding remove
button in the _listCode snippet (around lines 170-171) to maintain consistency
across both locations.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionConfig.razor`:
- Line 35: The guard for TransitionSpeed in the BmotionConfig.razor component
only checks if the value is negative but allows non-finite values like NaN and
Infinity to pass through, which can break downstream duration calculations.
Modify the assignment of _ctx.TransitionSpeed to validate not only that it is
non-negative but also that it is a finite number by using a method like
double.IsFinite() in addition to the negative check, defaulting to 0 for any
invalid values.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs`:
- Around line 17-22: The TransitionSpeed property currently accepts any double
value without validation, but negative speeds are invalid according to the
documented contract and will cause animations to malfunction. Add validation to
the TransitionSpeed property setter to reject negative values by throwing an
appropriate exception (such as ArgumentException or ArgumentOutOfRangeException)
when a negative value is assigned, ensuring that only non-negative values are
accepted.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cs`:
- Around line 17-19: The `Num(double)` method has a contract mismatch between
its XML documentation and implementation. The documentation states it provides
"full round-trip precision," but the implementation uses `"G6"` format which is
lossy and not full round-trip. Either replace the format string `"G6"` with
`"R"` (or `"G17"`) to achieve actual round-trip precision, or update the XML
summary comment to accurately describe the lossy `"G6"` compact formatting
behavior to match the intended implementation.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cs`:
- Around line 494-497: The current code validates only the first element with
`oa[0] is string`, but `Cast<string>()` will throw `InvalidCastException` if any
subsequent element is not a string. Fix this by adding a validation check that
ensures ALL elements in the `oa` array are strings before calling
`Cast<string>()`. Use `oa.All(x => x is string)` in addition to (or instead of)
the existing `oa[0] is string` check to validate the entire array before the
cast operation.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cs`:
- Around line 7-8: The XML documentation on the Axis property in
BmotionDragOptions contradicts the actual implementation. The property is
non-nullable with a default value of BmotionDragAxis.Both, not nullable. Update
the summary documentation for the Axis property to remove the reference to "Null
= both axes" and instead clarify that the property defaults to Both when not
explicitly set, reflecting the actual non-nullable contract of the property.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Line 47: Add a null guard at the start of the ObserveAsync method to validate
that the onChange parameter is not null. If onChange is null, throw an
ArgumentNullException with an appropriate parameter name. This prevents deferred
failures when the callback is invoked during scroll events by catching the issue
early at subscription time.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionValue.cs`:
- Around line 68-76: Both Subscribe method overloads accept null callbacks
without validation, allowing the failure to be deferred until notification time
instead of rejecting the invalid registration immediately. Add null validation
at the start of each Subscribe overload to throw ArgumentNullException if the
callback parameter is null. Apply this check to the async version
Subscribe(Func<T, Task> callback) and the synchronous convenience overload
Subscribe(Action<T> callback) before the callback is used or added to the
collection.
---
Duplicate comments:
In `@src/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cs`:
- Around line 169-176: The path drawing output contains broken dash math that
does not reliably produce progressive SVG path drawing, and this issue is
duplicated across two CSS emitter methods. In the ToCssStyleString method
(around lines 169-176 with stroke-dasharray and stroke-dashoffset calculations),
fix the normalized dash value calculations used in the stroke-dasharray and
stroke-dashoffset assignments to use correct math for progressive path drawing.
Then apply the same corrected formula to the duplicate code in
ToCssStyleDictionary method (around lines 229-236) which has the identical
stroke-dasharray and stroke-dashoffset calculations. Both methods must use
consistent and correct dash math to ensure reliable progressive SVG path drawing
behavior.
In `@src/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cs`:
- Around line 20-23: The EaseCubicBezier property currently lacks validation
despite its documentation requiring exactly 4 finite values in the format [x1,
y1, x2, y2]. Add validation logic to the EaseCubicBezier property setter (around
line 22) to ensure the assigned value is either null or an array with exactly 4
finite numeric values; reject any other input. Additionally, ensure the cloning
operation at line 129 also validates the array to maintain the invariant, so
invalid configs cannot persist through serialization or copying operations.
---
Nitpick comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor`:
- Around line 53-54: The OnDrag event handler fires continuously during drag
operations, causing redundant per-frame updates to _dragInfo with the
"Dragging…" status and unnecessary renders. Add an OnDragStart event binding to
the component and create a HandleDragStart method that sets _dragInfo to
"Dragging…" once at drag initiation. Remove or refactor the HandleDrag method to
eliminate the redundant status update, keeping only the necessary drag tracking
logic. Keep HandleDragEnd for setting the "Released" state when the drag
completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f06b4101-1a53-444c-9d4b-c29e7dde1965
📒 Files selected for processing (80)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
|
@coderabbitai re-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor`:
- Around line 46-54: The description text at line 46 mentions listening to
OnDragStart, OnDrag, and OnDragEnd events, but the Bmotion component only wires
callbacks for OnDragStart and OnDragEnd (the OnDrag callback is missing). Update
the description text to accurately reflect only the events that are actually
being listened to in the component. The same mismatch also exists at lines
145-153, so both locations need to be corrected to align the event descriptions
with the actual wired callbacks.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razor`:
- Around line 80-84: The AddEvent method continuously appends to the _events
list without any size limit, causing unbounded memory growth even though only
the last 8 entries are rendered to the UI. Add logic to the AddEvent method to
cap the _events list size—after adding a new event, check if the list exceeds a
reasonable maximum (likely 8 entries to match what's displayed) and remove older
entries from the beginning of the list to maintain a fixed size. This prevents
long-running sessions from accumulating unnecessary memory.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor`:
- Line 8: The ToString("F1") formatting at line 8 of ScrollAnimations.razor is
culture-sensitive and produces invalid CSS in locales with comma decimal
separators (e.g., "12,3%" instead of "12.3%"). Replace the ToString("F1") call
on the _progressY calculation with invariant culture formatting to ensure
CSS-valid output regardless of locale. Apply the same invariant culture
formatting fix to the identical pattern at line 86 in ScrollAnimations.razor to
maintain consistency across both locations where percentage widths are
dynamically calculated.
In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html`:
- Around line 37-47: The textarea cleanup code (document.body.removeChild(ta))
is only executed in the success path, so if ta.focus(), ta.select(), or
document.execCommand('copy') throws an exception, the textarea element remains
in the DOM. Restructure the try-catch block to include a finally block that
ensures document.body.removeChild(ta) always executes, guaranteeing cleanup
regardless of whether an exception occurs during the copy operation sequence.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cs`:
- Around line 64-76: The exit-transition bookkeeping in the condition checking
_prevIsPresent && !IsPresent has two issues: First, _presenceCtx.IsExiting is
set unconditionally even when BmotionPresenceContext.ChildCount is zero, causing
AllExitsComplete to never fire and _shouldRender to remain true indefinitely.
Second, _deferEnter is not cleared when entering exit mode, allowing stale
deferred-enter state to incorrectly persist and remount after a later exit
completes. Fix this by conditionally setting _presenceCtx.IsExiting only when
ChildCount > 0, and explicitly clear the _deferEnter flag when a new leave
occurs in the _prevIsPresent && !IsPresent branch.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cs`:
- Around line 21-30: The TransitionSpeed property setter currently only
validates that the value is non-negative but does not reject NaN or Infinity
values, which can propagate invalid timing values into animation math. Modify
the validation logic in the TransitionSpeed setter to also check that the value
is finite using double.IsFinite(), rejecting any non-finite values (NaN,
positive or negative Infinity) with an appropriate exception message alongside
the existing non-negative check.
In `@src/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cs`:
- Around line 9-33: The _nextChildIndex field continuously increments without
being reset, causing stagger delays computed in GetChildDelay to grow unbounded
in reused trees or subsequent variant cycles. Create a ResetChildrenOrder()
method that resets _nextChildIndex to 0, and ensure this method is called
whenever the parent component starts a fresh animation or variant cycle to
maintain correct per-cycle stagger ordering for RegisterChild() and
GetChildDelay().
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cs`:
- Around line 56-57: The regex patterns in the BmotionColorInterpolator class
are case-sensitive while the LooksLikeColor method accepts color functions
case-insensitively, causing uppercase inputs like RGB(...) or HSL(...) to be
detected but fail parsing. Add RegexOptions.IgnoreCase flag to the Regex.Match
calls in the Parse method for both the rgba pattern at lines 56-57 and the hsla
pattern at lines 70-71 to make them case-insensitive and properly handle color
inputs regardless of case.
- Around line 11-12: The Lerp method in BmotionColorInterpolator is parsing the
from and to color parameters on every frame tick, causing expensive regex
operations to repeat at ~60fps. Move the color parsing from the Lerp method to
the driver initialization phase in BmotionColorKeyframesDriver (around Line 62),
store the parsed color channel values, and modify Lerp to accept pre-parsed
channel data and perform only the interpolation calculation without any parsing
overhead.
In `@src/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cs`:
- Around line 28-30: The `frames` parameter is assigned directly to the
`_frames` field without cloning, which allows callers to mutate the original
array and affect the driver's behavior after construction. Clone the `frames`
parameter when assigning it to `_frames`, using the same cloning pattern already
applied to `_curFrames` (i.e., `_frames = (string[])frames.Clone();`), to keep
the driver's behavior deterministic and prevent external mutations from altering
`Cancel()` or `Complete()` outcomes.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs`:
- Around line 113-120: The issue is a race condition where multiple concurrent
AnimateAsync calls on the same element can cause premature UnregisterElement
calls while other animations are still active, combined with unsynchronized
access to the _elements dictionary in BmotionAnimationEngine. Fix this by
implementing reference counting: modify RegisterElement and UnregisterElement
methods to track the number of active animations per element (increment on
register, decrement on completion), and only actually unregister when the count
reaches zero. Additionally, synchronize all access to the _elements dictionary
by either wrapping all RegisterElement, UnregisterElement, and rAF loop
dictionary accesses with a lock, or replace the dictionary with
ConcurrentDictionary to ensure thread-safe mutations across the service and
engine components.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs`:
- Around line 32-49: Add null guards for the `props` parameter in the three
public methods AnimateAsync, AnimateAwaitAsync, and Set to prevent
NullReferenceException when null is passed as input. For each method, add a null
check for `props` at the beginning (similar to the existing check for
`_elementId`) and return early if it is null, ensuring the method does not
proceed to call `props.ToJsDictionary()` with a null value.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs`:
- Around line 62-63: The ObserveAsync overload that accepts an
Action<BmotionScrollInfo> onChange parameter does not validate that onChange is
not null before wrapping it in a lambda and passing it to the other ObserveAsync
overload. Add a null check at the beginning of this ObserveAsync method to throw
an ArgumentNullException if onChange is null, ensuring fail-fast validation
instead of allowing the null to be wrapped and cause a fault later when the
callback is actually invoked.
In `@src/Bmotion/README.md`:
- Line 1: Change the README title at line 1 from "# bit Bmotion" to "#
Bit.Bmotion" to align with the consistent package and namespace naming
convention used throughout the codebase.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs`:
- Around line 126-130: The cancel contract is inconsistent: tests assume
Cancel() snaps the value to the target, but the driver implementation treats
Cancel() as "complete immediately" without writing the target value on that
tick. In src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs at
lines 126-130, remove or modify the assertion Assert.AreEqual(100.0, lastValue,
1e-5) because Cancel() does not guarantee the lastValue reaches the target—only
that the operation completes; keep Assert.IsTrue(done) to validate completion.
Apply the same fix to
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cs at lines 111-115:
remove any assertion that expects the value to equal the target after Cancel(),
and retain only the assertion that validates completion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb1e4d1e-4049-4d59-98ae-f53c270c2d71
📒 Files selected for processing (80)
src/Bmotion/Bit.Bmotion.Demos/App.razorsrc/Bmotion/Bit.Bmotion.Demos/Bit.Bmotion.Demos.csprojsrc/Bmotion/Bit.Bmotion.Demos/Layout/MainLayout.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/AnimatePresencePage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/BasicAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Home.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Keyframes.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/LayoutPage.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Springs.razorsrc/Bmotion/Bit.Bmotion.Demos/Pages/Variants.razorsrc/Bmotion/Bit.Bmotion.Demos/Program.cssrc/Bmotion/Bit.Bmotion.Demos/Properties/launchSettings.jsonsrc/Bmotion/Bit.Bmotion.Demos/Shared/CodeSnippet.razorsrc/Bmotion/Bit.Bmotion.Demos/Shared/SyntaxHighlighter.cssrc/Bmotion/Bit.Bmotion.Demos/_Imports.razorsrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/app.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/css/motion-samples.csssrc/Bmotion/Bit.Bmotion.Demos/wwwroot/index.htmlsrc/Bmotion/Bit.Bmotion.slnxsrc/Bmotion/Bit.Bmotion/Bit.Bmotion.csprojsrc/Bmotion/Bit.Bmotion/BitBmotion.cssrc/Bmotion/Bit.Bmotion/Components/Bmotion.cssrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razorsrc/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cssrc/Bmotion/Bit.Bmotion/Components/BmotionConfig.razorsrc/Bmotion/Bit.Bmotion/Context/BmotionConfigContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionPresenceContext.cssrc/Bmotion/Bit.Bmotion/Context/BmotionVariantContext.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorInterpolator.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionColorTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionCssFormat.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionEasingFunctions.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionElementAnimationState.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionInertiaDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionNumericKeyframesDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionSpringDriver.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTransformComposer.cssrc/Bmotion/Bit.Bmotion/Engine/BmotionTweenDriver.cssrc/Bmotion/Bit.Bmotion/Engine/IBmotionAnimationDriver.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionBoundingRect.cssrc/Bmotion/Bit.Bmotion/Interop/BmotionInterop.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationProps.cssrc/Bmotion/Bit.Bmotion/Models/BmotionAnimationTarget.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragAxis.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragConstraints.cssrc/Bmotion/Bit.Bmotion/Models/BmotionDragOptions.cssrc/Bmotion/Bit.Bmotion/Models/BmotionEasing.cssrc/Bmotion/Bit.Bmotion/Models/BmotionMotionVariants.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPanInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionPointInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionRepeatType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionScrollInfo.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionConfig.cssrc/Bmotion/Bit.Bmotion/Models/BmotionTransitionType.cssrc/Bmotion/Bit.Bmotion/Models/BmotionViewportOptions.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cssrc/Bmotion/Bit.Bmotion/Services/BmotionAnimationControls.cssrc/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValue.cssrc/Bmotion/Bit.Bmotion/Services/BmotionValueFactory.cssrc/Bmotion/Bit.Bmotion/_Imports.razorsrc/Bmotion/Bit.Bmotion/wwwroot/bit-bmotion.jssrc/Bmotion/README.mdsrc/Bmotion/Tests/Bit.Bmotion.Tests/Bit.Bmotion.Tests.csprojsrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorInterpolatorTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/ColorTweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/EasingFunctionsTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/InertiaDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/KeyframesDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TransformComposerTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/GlobalUsings.cssrc/Bmotion/Tests/Bit.Bmotion.Tests/Models/TransitionConfigTests.cs
| <p>Listen to <c>OnDragStart</c>, <c>OnDrag</c>, <c>OnDragEnd</c> for real-time position feedback.</p> | ||
| <div class="demo-row"> | ||
| <div class="demo-card" style="flex-direction:column;gap:1rem;position:relative;overflow:hidden;height:220px;"> | ||
| <Bmotion Tag="div" | ||
| Class="box box-sm" | ||
| Style="cursor:grab" | ||
| Drag="true" | ||
| OnDragStart="@HandleDragStart" | ||
| OnDragEnd="@HandleDragEnd" /> |
There was a problem hiding this comment.
Align the Drag Events description with the wired callbacks.
Line 46 says this sample listens to OnDrag, but the demo only wires OnDragStart/OnDragEnd (Lines 53-54). The snippet text at Line 145 has the same mismatch.
Suggested patch
- <p>Listen to <c>OnDragStart</c>, <c>OnDrag</c>, <c>OnDragEnd</c> for real-time position feedback.</p>
+ <p>Listen to <c>OnDragStart</c> and <c>OnDragEnd</c> for drag lifecycle feedback.</p>- <p>Listen to <c>OnDragStart</c>, <c>OnDrag</c>, <c>OnDragEnd</c> for real-time position feedback.</p>
+ <p>Listen to <c>OnDragStart</c> and <c>OnDragEnd</c> for drag lifecycle feedback.</p>Also applies to: 145-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/DragPage.razor` around lines 46 - 54, The
description text at line 46 mentions listening to OnDragStart, OnDrag, and
OnDragEnd events, but the Bmotion component only wires callbacks for OnDragStart
and OnDragEnd (the OnDrag callback is missing). Update the description text to
accurately reflect only the events that are actually being listened to in the
component. The same mismatch also exists at lines 145-153, so both locations
need to be corrected to align the event descriptions with the actual wired
callbacks.
| void AddEvent(string name) | ||
| { | ||
| _events.Add($"[{DateTime.Now:HH:mm:ss.ff}] {name}"); | ||
| StateHasChanged(); | ||
| } |
There was a problem hiding this comment.
Bound the in-memory event log to prevent unbounded growth.
_events keeps growing while only the last 8 entries are rendered. Cap the list size to avoid long-session memory/perf drift.
Suggested patch
void AddEvent(string name)
{
_events.Add($"[{DateTime.Now:HH:mm:ss.ff}] {name}");
+ const int maxEvents = 200;
+ if (_events.Count > maxEvents)
+ {
+ _events.RemoveRange(0, _events.Count - maxEvents);
+ }
StateHasChanged();
}Also applies to: 213-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/Gestures.razor` around lines 80 - 84, The
AddEvent method continuously appends to the _events list without any size limit,
causing unbounded memory growth even though only the last 8 entries are rendered
to the UI. Add logic to the AddEvent method to cap the _events list size—after
adding a new event, check if the list exceeds a reasonable maximum (likely 8
entries to match what's displayed) and remove older entries from the beginning
of the list to maintain a fixed size. This prevents long-running sessions from
accumulating unnecessary memory.
| <div style="position:sticky;top:3.25rem;z-index:50;background:var(--bm-card);border-radius:12px;padding:1.5rem;margin-bottom:1.5rem;box-shadow:0 4px 20px rgba(0,0,0,.35)"> | ||
| <div style="font-size:.9rem;color:#888;margin-bottom:.5rem">Window scroll progress</div> | ||
| <div style="background:#111;border-radius:6px;height:8px;overflow:hidden;width:100%"> | ||
| <div style="@($"height:100%;border-radius:6px;background:linear-gradient(90deg,#6c47ff,#ff4785);width:{(_progressY * 100).ToString("F1")}%;transition:width .05s")" /> |
There was a problem hiding this comment.
Use invariant formatting for CSS numeric style values.
Line 8 formats width with ToString("F1"), which is culture-sensitive. In locales with comma decimals, the generated CSS width can be invalid (e.g., 12,3%). Mirror this fix in the snippet sample at Line 86.
Suggested patch
- <div style="@($"height:100%;border-radius:6px;background:linear-gradient(90deg,`#6c47ff`,`#ff4785`);width:{(_progressY * 100).ToString("F1")}%;transition:width .05s")" />
+ <div style="@($"height:100%;border-radius:6px;background:linear-gradient(90deg,`#6c47ff`,`#ff4785`);width:{(_progressY * 100).ToString("F1", System.Globalization.CultureInfo.InvariantCulture)}%;transition:width .05s")" />- <div style="@($"height:100%;border-radius:6px;background:linear-gradient(90deg,`#6c47ff`,`#ff4785`);width:{(_progressY * 100).ToString("F1")}%;transition:width .05s")" />
+ <div style="@($"height:100%;border-radius:6px;background:linear-gradient(90deg,`#6c47ff`,`#ff4785`);width:{(_progressY * 100).ToString("F1", System.Globalization.CultureInfo.InvariantCulture)}%;transition:width .05s")" />Also applies to: 86-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion.Demos/Pages/ScrollAnimations.razor` at line 8, The
ToString("F1") formatting at line 8 of ScrollAnimations.razor is
culture-sensitive and produces invalid CSS in locales with comma decimal
separators (e.g., "12,3%" instead of "12.3%"). Replace the ToString("F1") call
on the _progressY calculation with invariant culture formatting to ensure
CSS-valid output regardless of locale. Apply the same invariant culture
formatting fix to the identical pattern at line 86 in ScrollAnimations.razor to
maintain consistency across both locations where percentage widths are
dynamically calculated.
| try { | ||
| const ta = document.createElement('textarea'); | ||
| ta.value = text; | ||
| ta.style.position = 'fixed'; | ||
| ta.style.opacity = '0'; | ||
| document.body.appendChild(ta); | ||
| ta.focus(); | ||
| ta.select(); | ||
| const ok = document.execCommand('copy'); | ||
| document.body.removeChild(ta); | ||
| return ok; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -60 src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html | tail -30Repository: bitfoundation/bitplatform
Length of output: 1048
Ensure fallback textarea cleanup runs even when copy operations throw.
If ta.focus(), ta.select(), or document.execCommand('copy') throws an exception, the catch block at line 50 returns false without executing removeChild. This leaves hidden <textarea> nodes in the DOM, and repeated failures accumulate them. Move cleanup to a finally block.
Suggested fix
- try {
- const ta = document.createElement('textarea');
- ta.value = text;
- ta.style.position = 'fixed';
- ta.style.opacity = '0';
- document.body.appendChild(ta);
- ta.focus();
- ta.select();
- const ok = document.execCommand('copy');
- document.body.removeChild(ta);
- return ok;
- } catch {
- return false;
- }
+ let ta;
+ try {
+ ta = document.createElement('textarea');
+ ta.value = text;
+ ta.style.position = 'fixed';
+ ta.style.opacity = '0';
+ document.body.appendChild(ta);
+ ta.focus();
+ ta.select();
+ return document.execCommand('copy');
+ } catch {
+ return false;
+ } finally {
+ if (ta && ta.parentNode) ta.parentNode.removeChild(ta);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const ta = document.createElement('textarea'); | |
| ta.value = text; | |
| ta.style.position = 'fixed'; | |
| ta.style.opacity = '0'; | |
| document.body.appendChild(ta); | |
| ta.focus(); | |
| ta.select(); | |
| const ok = document.execCommand('copy'); | |
| document.body.removeChild(ta); | |
| return ok; | |
| let ta; | |
| try { | |
| ta = document.createElement('textarea'); | |
| ta.value = text; | |
| ta.style.position = 'fixed'; | |
| ta.style.opacity = '0'; | |
| document.body.appendChild(ta); | |
| ta.focus(); | |
| ta.select(); | |
| return document.execCommand('copy'); | |
| } catch { | |
| return false; | |
| } finally { | |
| if (ta && ta.parentNode) ta.parentNode.removeChild(ta); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion.Demos/wwwroot/index.html` around lines 37 - 47, The
textarea cleanup code (document.body.removeChild(ta)) is only executed in the
success path, so if ta.focus(), ta.select(), or document.execCommand('copy')
throws an exception, the textarea element remains in the DOM. Restructure the
try-catch block to include a finally block that ensures
document.body.removeChild(ta) always executes, guaranteeing cleanup regardless
of whether an exception occurs during the copy operation sequence.
| if (_prevIsPresent && !IsPresent) | ||
| { | ||
| // Children are leaving - signal exiting state so Bmotion components play Exit | ||
| _presenceCtx.IsExiting = true; | ||
| _shouldRender = true; // keep rendering until exit completes | ||
| } | ||
| else if (!_prevIsPresent && IsPresent) | ||
| { | ||
| if (ExitBeforeEnter && _presenceCtx.IsExiting) | ||
| { | ||
| // Wait for the exiting children to finish before rendering the new ones. | ||
| _deferEnter = true; | ||
| } |
There was a problem hiding this comment.
Fix exit-transition bookkeeping for zero-child and rapid-toggle paths.
At Line 64, exit mode is entered unconditionally. If BmotionPresenceContext.ChildCount == 0, AllExitsComplete never fires and _shouldRender can stay true after IsPresent=false. Also, _deferEnter is not cleared on a new leave, so stale deferred-enter state can incorrectly remount after a later exit completes.
Suggested minimal fix
if (_prevIsPresent && !IsPresent)
{
- // Children are leaving - signal exiting state so Bmotion components play Exit
- _presenceCtx.IsExiting = true;
- _shouldRender = true; // keep rendering until exit completes
+ _deferEnter = false; // clear stale deferred-enter intent on a fresh leave
+ if (_presenceCtx.ChildCount == 0)
+ {
+ _presenceCtx.IsExiting = false;
+ _shouldRender = false; // no exit participants -> unmount immediately
+ }
+ else
+ {
+ _presenceCtx.IsExiting = true;
+ _shouldRender = true; // keep rendering until exit completes
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (_prevIsPresent && !IsPresent) | |
| { | |
| // Children are leaving - signal exiting state so Bmotion components play Exit | |
| _presenceCtx.IsExiting = true; | |
| _shouldRender = true; // keep rendering until exit completes | |
| } | |
| else if (!_prevIsPresent && IsPresent) | |
| { | |
| if (ExitBeforeEnter && _presenceCtx.IsExiting) | |
| { | |
| // Wait for the exiting children to finish before rendering the new ones. | |
| _deferEnter = true; | |
| } | |
| if (_prevIsPresent && !IsPresent) | |
| { | |
| _deferEnter = false; // clear stale deferred-enter intent on a fresh leave | |
| if (_presenceCtx.ChildCount == 0) | |
| { | |
| _presenceCtx.IsExiting = false; | |
| _shouldRender = false; // no exit participants -> unmount immediately | |
| } | |
| else | |
| { | |
| _presenceCtx.IsExiting = true; | |
| _shouldRender = true; // keep rendering until exit completes | |
| } | |
| } | |
| else if (!_prevIsPresent && IsPresent) | |
| { | |
| if (ExitBeforeEnter && _presenceCtx.IsExiting) | |
| { | |
| // Wait for the exiting children to finish before rendering the new ones. | |
| _deferEnter = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion/Components/BmotionAnimatePresence.razor.cs` around
lines 64 - 76, The exit-transition bookkeeping in the condition checking
_prevIsPresent && !IsPresent has two issues: First, _presenceCtx.IsExiting is
set unconditionally even when BmotionPresenceContext.ChildCount is zero, causing
AllExitsComplete to never fire and _shouldRender to remain true indefinitely.
Second, _deferEnter is not cleared when entering exit mode, allowing stale
deferred-enter state to incorrectly persist and remount after a later exit
completes. Fix this by conditionally setting _presenceCtx.IsExiting only when
ChildCount > 0, and explicitly clear the _deferEnter flag when a new leave
occurs in the _prevIsPresent && !IsPresent branch.
| if (ours.Count > 0) | ||
| { | ||
| // Release engine state for the elements we created once their animations settle. | ||
| _ = completion.ContinueWith(_ => | ||
| { | ||
| foreach (var id in ours) | ||
| _engine.UnregisterElement(id); | ||
| }, TaskScheduler.Default); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current cleanup path and scheduler usage in animate service
rg -n -C3 'ContinueWith|TaskScheduler\.Default|UnregisterElement|RegisterElement' src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs
# Verify engine state storage/synchronization signals around element table access
rg -n -C3 '_elements|RegisterElement|UnregisterElement|IsRegistered|lock\s*\(' src/Bmotion/Bit.Bmotion/Engine/BmotionAnimationEngine.csRepository: bitfoundation/bitplatform
Length of output: 6744
Overlapping animations on the same element race to unregister state.
When two AnimateAsync calls target the same element concurrently, the earlier completion will unconditionally UnregisterElement (line 119) while the later call may still be active. More critically, the _elements dictionary in BmotionAnimationEngine has no synchronization: RegisterElement, UnregisterElement, and the rAF loop (line 312) all access it directly without locks. Using TaskScheduler.Default executes cleanup on a thread pool thread, risking mutation during the rAF loop's iteration on another thread.
Implement reference counting (increment per service-call register, decrement on completion, unregister at zero) and synchronize all _elements access via a single lock or use ConcurrentDictionary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimateService.cs` around lines 113 -
120, The issue is a race condition where multiple concurrent AnimateAsync calls
on the same element can cause premature UnregisterElement calls while other
animations are still active, combined with unsynchronized access to the
_elements dictionary in BmotionAnimationEngine. Fix this by implementing
reference counting: modify RegisterElement and UnregisterElement methods to
track the number of active animations per element (increment on register,
decrement on completion), and only actually unregister when the count reaches
zero. Additionally, synchronize all access to the _elements dictionary by either
wrapping all RegisterElement, UnregisterElement, and rAF loop dictionary
accesses with a lock, or replace the dictionary with ConcurrentDictionary to
ensure thread-safe mutations across the service and engine components.
| public async ValueTask AnimateAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null) | ||
| { | ||
| if (_elementId == null) return; | ||
| await _engine.AnimateToAsync(_elementId, props.ToJsDictionary(), transition); | ||
| } | ||
|
|
||
| /// <summary>Animate and await completion.</summary> | ||
| public async ValueTask AnimateAwaitAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null) | ||
| { | ||
| if (_elementId == null) return; | ||
| await _engine.AnimateToAwaitAsync(_elementId, props.ToJsDictionary(), transition); | ||
| } | ||
|
|
||
| /// <summary>Instantly set props without animation.</summary> | ||
| public void Set(BmotionAnimationProps props) | ||
| { | ||
| if (_elementId == null) return; | ||
| _engine.SetInstant(_elementId, props.ToJsDictionary()); |
There was a problem hiding this comment.
Add null guards for props on public API methods.
AnimateAsync, AnimateAwaitAsync, and Set call props.ToJsDictionary() directly. A null caller input will throw a less actionable NRE.
Suggested fix
public async ValueTask AnimateAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null)
{
+ ArgumentNullException.ThrowIfNull(props);
if (_elementId == null) return;
await _engine.AnimateToAsync(_elementId, props.ToJsDictionary(), transition);
}
public async ValueTask AnimateAwaitAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null)
{
+ ArgumentNullException.ThrowIfNull(props);
if (_elementId == null) return;
await _engine.AnimateToAwaitAsync(_elementId, props.ToJsDictionary(), transition);
}
public void Set(BmotionAnimationProps props)
{
+ ArgumentNullException.ThrowIfNull(props);
if (_elementId == null) return;
_engine.SetInstant(_elementId, props.ToJsDictionary());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public async ValueTask AnimateAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null) | |
| { | |
| if (_elementId == null) return; | |
| await _engine.AnimateToAsync(_elementId, props.ToJsDictionary(), transition); | |
| } | |
| /// <summary>Animate and await completion.</summary> | |
| public async ValueTask AnimateAwaitAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null) | |
| { | |
| if (_elementId == null) return; | |
| await _engine.AnimateToAwaitAsync(_elementId, props.ToJsDictionary(), transition); | |
| } | |
| /// <summary>Instantly set props without animation.</summary> | |
| public void Set(BmotionAnimationProps props) | |
| { | |
| if (_elementId == null) return; | |
| _engine.SetInstant(_elementId, props.ToJsDictionary()); | |
| public async ValueTask AnimateAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null) | |
| { | |
| ArgumentNullException.ThrowIfNull(props); | |
| if (_elementId == null) return; | |
| await _engine.AnimateToAsync(_elementId, props.ToJsDictionary(), transition); | |
| } | |
| /// <summary>Animate and await completion.</summary> | |
| public async ValueTask AnimateAwaitAsync(BmotionAnimationProps props, BmotionTransitionConfig? transition = null) | |
| { | |
| ArgumentNullException.ThrowIfNull(props); | |
| if (_elementId == null) return; | |
| await _engine.AnimateToAwaitAsync(_elementId, props.ToJsDictionary(), transition); | |
| } | |
| /// <summary>Instantly set props without animation.</summary> | |
| public void Set(BmotionAnimationProps props) | |
| { | |
| ArgumentNullException.ThrowIfNull(props); | |
| if (_elementId == null) return; | |
| _engine.SetInstant(_elementId, props.ToJsDictionary()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionAnimationController.cs` around lines
32 - 49, Add null guards for the `props` parameter in the three public methods
AnimateAsync, AnimateAwaitAsync, and Set to prevent NullReferenceException when
null is passed as input. For each method, add a null check for `props` at the
beginning (similar to the existing check for `_elementId`) and return early if
it is null, ensuring the method does not proceed to call
`props.ToJsDictionary()` with a null value.
| public Task ObserveAsync(string? containerId, Action<BmotionScrollInfo> onChange) | ||
| => ObserveAsync(containerId, info => { onChange(info); return Task.CompletedTask; }); |
There was a problem hiding this comment.
Validate Action callback in the synchronous ObserveAsync overload.
A null onChange here is wrapped into a non-null lambda, bypassing fail-fast validation and later faulting inside the callback path.
Suggested fix
public Task ObserveAsync(string? containerId, Action<BmotionScrollInfo> onChange)
- => ObserveAsync(containerId, info => { onChange(info); return Task.CompletedTask; });
+{
+ ArgumentNullException.ThrowIfNull(onChange);
+ return ObserveAsync(containerId, info => { onChange(info); return Task.CompletedTask; });
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Bit.Bmotion/Services/BmotionScrollTracker.cs` around lines 62 -
63, The ObserveAsync overload that accepts an Action<BmotionScrollInfo> onChange
parameter does not validate that onChange is not null before wrapping it in a
lambda and passing it to the other ObserveAsync overload. Add a null check at
the beginning of this ObserveAsync method to throw an ArgumentNullException if
onChange is null, ensuring fail-fast validation instead of allowing the null to
be wrapped and cause a fault later when the callback is actually invoked.
| @@ -0,0 +1,383 @@ | |||
| # bit Bmotion | |||
There was a problem hiding this comment.
Normalize the README title to the package branding.
At Line 1, # bit Bmotion is inconsistent with the package/namespace naming used throughout (Bit.Bmotion), which can hurt discoverability and consistency.
Suggested patch
-# bit Bmotion
+# Bit.Bmotion📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # bit Bmotion | |
| # Bit.Bmotion |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/README.md` at line 1, Change the README title at line 1 from "#
bit Bmotion" to "# Bit.Bmotion" to align with the consistent package and
namespace naming convention used throughout the codebase.
| driver.Cancel(); | ||
| bool done = driver.Tick(32); | ||
|
|
||
| Assert.AreEqual(100.0, lastValue, 1e-5); | ||
| Assert.IsTrue(done); |
There was a problem hiding this comment.
Cancel contract is inconsistent between tests and current driver implementations.
Both tests assume “cancel snaps to target,” but current driver logic treats cancel as “complete immediately” without applying the target on that tick.
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs#L126-L130: update the assertion to validate completion without expectinglastValueto jump to target (or explicitly callComplete()if snap semantics are intended).src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cs#L111-L115: same contract alignment—assert completion without expecting a target write unlessComplete()is called.
📍 Affects 2 files
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs#L126-L130(this comment)src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cs#L111-L115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs` around lines
126 - 130, The cancel contract is inconsistent: tests assume Cancel() snaps the
value to the target, but the driver implementation treats Cancel() as "complete
immediately" without writing the target value on that tick. In
src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/SpringDriverTests.cs at lines
126-130, remove or modify the assertion Assert.AreEqual(100.0, lastValue, 1e-5)
because Cancel() does not guarantee the lastValue reaches the target—only that
the operation completes; keep Assert.IsTrue(done) to validate completion. Apply
the same fix to src/Bmotion/Tests/Bit.Bmotion.Tests/Engine/TweenDriverTests.cs
at lines 111-115: remove any assertion that expects the value to equal the
target after Cancel(), and retain only the assertion that validates completion.
closes #12447
Summary by CodeRabbit